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

Extract open issues and organize Expressions section #8209

Merged
merged 11 commits into from
Jul 15, 2024

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Jun 12, 2024

Relates to test plan dotnet/roslyn#66722

@jcouv jcouv self-assigned this Jun 12, 2024
@@ -601,41 +760,61 @@ The preceding rules mean:
- and that extension methods declared directly in a namespace take precedence
over extension methods imported into that same namespace with a using namespace directive.

TODO(static) clarify behavior for extension on `object` or `dynamic` used as `dynamic.M()`?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 This is converted to a test with a PROTOTYPE comment about diagnostic quality in PR

@jcouv jcouv marked this pull request as ready for review June 24, 2024 20:55
@jcouv jcouv requested a review from a team as a code owner June 24, 2024 20:55
@jcouv jcouv requested a review from AlekseyTs June 25, 2024 15:52
Copy link
Contributor

@MadsTorgersen MadsTorgersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good restructuring! Makes it a lot easier to get an overview on the current state of design, and which are the most pressing issues.


Will need to check with the WG as I don't recall the reasoning for this.

## Open issue: need to specify the type erasure design
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between this and "Open issue: need to specify type erasure" above?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged now. Thanks

An *element_access* is dynamically bound if \[...]
If the *primary_no_array_creation_expression* of an *element_access* is a value of an *array_type*, the *element_access* is an array access.
Otherwise, the *primary_no_array_creation_expression* shall be a variable or value of a class, struct, or interface type TODOTODO
Otherwise, the *primary_no_array_creation_expression* shall be a variable or value of a class, struct, interface, /***or extension** type
Copy link
Member

@jjonescz jjonescz Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to write \* instead of /* ? But I don't know why would that be needed, this seems to work fine:

Suggested change
Otherwise, the *primary_no_array_creation_expression* shall be a variable or value of a class, struct, interface, /***or extension** type
Otherwise, the *primary_no_array_creation_expression* shall be a variable or value of a class, struct, interface, **or extension** type

Applies to other similar changes throughout the doc - I see usages of /* and /[ that were probably meant to be \* and \[ (although the escaping might be actually unnecessary).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried again and it looks like the escape sequence is no longer needed. Removed :-)

@@ -500,7 +497,7 @@ We modify the [accessibility constraints](https://github.com/dotnet/csharpstanda

The following accessibility constraints exist:
- [...]
- \***The underlying type of an extension type shall be at least as accessible as the extension type itself.**
- ***The underlying type of an extension type shall be at least as accessible as the extension type itself.**
Copy link
Member

@jjonescz jjonescz Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it seems excessive to use three * - the first is rendered as a literal * and the remaining two make the text bold (at least in the GitHub markdown renderer):

image

Or was that intended?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intended. If found that the bold is not always easy to see, so the * helps spot additions even when the bold isn't.
It looks like GitHub may have adjusted their fonts and the bold seems easier to see than before, but I think it's better to keep for now. It'll be easy to remove if we decide this extra formatting isn't necessary.

@jcouv
Copy link
Member Author

jcouv commented Jun 27, 2024

@AlekseyTs for review. Thanks


## Open issue: removing zero-arity-matches-any from var scenario

By the current rules, there are some unfortunate interactions between member access and natural function type.
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are some unfortunate interactions between member access and natural function type

This is not specific. What interactions? Why might one consider them unfortunate? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the example below (line 142). A member that cannot work in the end gets in the way. We've taken steps in overload resolution and more recently in natural type to reduce such behaviors (discard bad candidates earlier).
I'm not sure we should do something here, maybe the current behavior is desirable fallout of current rules...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to remove this open issue. I don't have a good proposal for how to make this scenario work and I'm no longer convinced it should.

Here's an updated summary of the question for the record:

Open issue: removing zero-arity-matches-any from var scenario

By the current rules, a generic method which cannot possibly work in a function type scenario can hide
a member that would work:

var x = C.Member; // error: member lookup finds C.Member (method group) and lacks type arguments to apply to that match

class C
{
    public static void Member<T>() { }
}

implicit extension E for C
{
    public static int Member = 42;
}

Is this a scenario we think should work?

For context, from the member access rules,
we perform a member lookup which includes generic methods even when no type arguments are present:

[...] if a member lookup of I in E with K type parameters produces a match, then E.I is evaluated and classified as follows
Note: When the result of such a member lookup is a method group and K is zero,
the method group can contain methods having type parameters.
This allows such methods to be considered for type argument inferencing. end note

See details from the member lookup rules:

When K is zero, methods having type parameters are not removed, since the type inference process might be able to infer the type arguments.

See details on natural type of method groups

## Open issue: removing zero-arity-matches-any from var scenario

By the current rules, there are some unfortunate interactions between member access and natural function type.
> Note: When the result of such a member lookup is a method group and K is zero,
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not obvious what is the context for the quote (where it is taken from) and how it is relevant to the issue. #Closed

> the method group can contain methods having type parameters.
> This allows such methods to be considered for type argument inferencing. end note

I would like to brainstorm tweaks to member access and natural function type to make this work better.
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better is a subjective term, consider describing the behavior that you think is better. #Closed

I would like to brainstorm tweaks to member access and natural function type to make this work better.

```csharp
var x = C.Member; // error: member lookup finds C.Member (method group) and lacks type arguments to apply to that match
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might help including what does the error say exactly #Closed

where `P` is an instance of an extension type on a nullable value type,
- the spec for an object creation considers whether the type is
a value_type, a type_parameter, a class_type or a struct_type,
- the spec for satisfying constraints also consider what kind of type were dealing with.
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo? "what kind of type were dealing with" #Closed

## Open issue: need to specify type erasure

It should be done in a way that allows switching between the extension type and the underlying type without binary break.
It should allow for using type parameters as type parameters: `void M<T>(E<T> e)`.
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to say: "It should allow for using type parameters as type arguments" ? #Closed

The current proposal is to use an attribute with a string representing the type with extensions un-erased.
For example: `void M(E)` would be emitted as `void M([Attribute("E")] UnderlyingType)`.

The serialization format would be the same as the one used for `typeof` in attributes, but with support for type parameters (using `T!` and `T!!` syntax).
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the proposal was to follow the way specified in ECMA-335 (i.e. to refer by ordinal, rather than by name):

II.9 Generics
Within a generic type definition, its generic parameters are referred to by their index. Generic
parameter zero is referred to as !0, generic parameter one as !1, and so on. Similarly, within the body
of a generic method definition, its generic parameters are referred to by their index; generic parameter
zero is referred to as !!0, generic parameter one as !!1, and so on. #Closed

Note: We allow static lookups on type parameters only for members that are static virtual members.
Since extensions members are not virtual, we don't allow static extension lookups on type parameters either.
```
void M<T>(T t) where T
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constraint is not finished. Did you mean "where T : U"? #Closed

- ***When this is used in a primary_expression within an instance method or instance accessor of an extension with a class underlying type,
it is classified as a value. The type of the value is the instance type of the extension within which the usage occurs,
and the value is a reference to the object for which the method or accessor was invoked.**
- ***When this is used in a primary_expression within an instance method or instance accessor of an extension with a struct underlying type,
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to specify the meaning for a scenario when the underlying type is a type parameter not known to be a reference type or a value type. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I meant to have this as an open issue but forgot. Added.


A this_access has one of the following meanings:
- [...]
- ***When this is used in a primary_expression within an instance method or instance accessor of an extension with a class underlying type,
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a class underlying type

A class underlying type, or an underlying type known to be a reference type? For example, an interface type is not a class and is not a struct, and is not a type parameter. #Closed

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 10)

@jcouv jcouv requested a review from AlekseyTs July 2, 2024 18:17
@jcouv
Copy link
Member Author

jcouv commented Jul 9, 2024

@AlekseyTs Please take another look. Thanks

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 11)

@jcouv jcouv merged commit 98971c2 into dotnet:main Jul 15, 2024
1 check passed
@jcouv jcouv deleted the roles-instances branch July 15, 2024 17:18
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

Successfully merging this pull request may close these issues.

4 participants