Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open issue: non-negative assumption should apply to both Count and Length #5137

Closed
jcouv opened this issue Sep 2, 2021 · 13 comments
Closed
Assignees

Comments

@jcouv
Copy link
Member

jcouv commented Sep 2, 2021

Should the non-negative assumption apply to the used Property or both?

The current behavior is shown below. I think we should have an error for both lines.

interface IIndexable
{
    int this[int i] { get; }
}
interface ICountableViaCount
{
    int Count { get; }
}
interface ICountableViaLength
{
    int Length { get; }
}
class X
{
    public static void Test5<T>(T t) where T : IIndexable, ICountableViaLength, ICountableViaCount
    {
        _ = t is { Length: -1 }; // error CS8518: An expression of type 'T' can never match the provided pattern.
        _ = t is { Count: -1 }; // should Count benefit from the assumption when it is not the preferred property?
    }
}

Should we assume that Length and Count have the same value?

Another related question is whether we should assume Length and Count to have the same value? For example: case { Length: > 0 }: case { Count: 0 }: case { Count: var unreachable }:.

Should we assume non-null slice when we expect a non-zero slice?

Should we consider null or { Length: not 4 } => 0, [_, .. [_, _], _] => 0 exhaustive in a nullable context?
It is possible for a type to claim Length=4 but return a null slice, but that would violate our assumption that lengths and elements are consistent between outer and nested lists.

Options:

  1. this switch is not exhaustive: this is simpler, safer, but goes slightly against our "well-behaved collection" assumption
  2. this switch is exhaustive assuming a well-behaved collection: not sure we want to modify our DAG/codegen for this
  3. reject this slice method because it returns a nullable type: this seems odd, as we generally don't want nullability to affect semantics

FYI @alrz @333fred

Relates to proposal #3435
Relates to test plan dotnet/roslyn#51289

@JustNrik
Copy link

JustNrik commented Sep 2, 2021

Something I've always been curious about, why are Length and Count declared as int when they are really never going to be negative? like, shouldn't they be uint instead?

Also, why have two interfaces that are essentially the same when you could just have a single ICountable interface, or perhaps ISizable with Size property?

Should IIndexable really be readonly? couldn't we have IReadOnlyIndexable with IIndexable : IReadOnlyIndexable instead? Or are all of this interfaces just for list patterns features and not general abstraction?

@CyrusNajmabadi
Copy link
Member

Unsigned integers were not cls compliant.

@theunrepentantgeek
Copy link

When the compiler encounters this line will it already be sure that it's working with a collection or sequence?

 _ = t is { Length: -1 }; // error CS8518: An expression of type 'T' can never match the provided pattern.

If the compiler can't be completely sure that t is an array (or a similar container) then a warning here might be a problem due to differing semantics. Not every Length is strictly non-negative.

@CyrusNajmabadi
Copy link
Member

Our position is that it's fine for the language to assume these types are well behaved for the purposes of pattern matching.

@FaustVX
Copy link

FaustVX commented Sep 2, 2021

Why not put an attribute on the correct length property (like IndexableLength)
This will allow to have any name for the property we want (Count, Length, Size, even other language Longueur)
Maybe with a parameter to allow negative value.

class C {
  public object this[int i] { get; }
  [IndexableLength(allowNegative = false)]
  public int Size { get; }
}

_ = new C() is { Size: -1 }; // error

@CyrusNajmabadi
Copy link
Member

Why not put an attribute on the correct length property (like IndexableLength)

Because the APIs have already shipped, and we'd want patterns to work across the enormous ecosystem of well-behaving types already out there.

@333fred
Copy link
Member

333fred commented Sep 2, 2021

When the compiler encounters this line will it already be sure that it's working with a collection or sequence?

Yes, it will only make this assumption when the type is both Countable and Indexable, where those words come from the index and range feature in C# 8.

@alrz
Copy link
Member

alrz commented Sep 9, 2021

I suppose LongLength would be out of scope here because it doesn't have any special meaning.

@jcouv
Copy link
Member Author

jcouv commented Oct 19, 2021

FYI, I added a third question to OP for Wednesday:
Should we consider null or { Length: not 4 } => 0, [_, .. [_, _], _] => 0 exhaustive in a nullable-enabled context?

@jcouv
Copy link
Member Author

jcouv commented Oct 20, 2021

This was discussed in LDM 10/20/2021. In short:

  1. only Length or Count gets the benefit of the non-negative assumption
  2. Length and Count are not assumed to be consistent
  3. produce an exhaustiveness diagnostic
    All the above are subject to revision based on user feedback.

@alrz
Copy link
Member

alrz commented Oct 20, 2021

  1. only Length or Count gets the benefit of the non-negative assumption

Does this mean:

  • If both are present, only one of them is assumed to be non-negative, or
  • We won't consider anything else like LongLength in this assumption.

@jcouv
Copy link
Member Author

jcouv commented Oct 20, 2021

The former "If both are present, only one of them is assumed to be non-negative". If there is a Length, then we say/assume nothing about Count.
I realize that's what you'd implemented initially. Sorry for the reversal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants