csharplang icon indicating copy to clipboard operation
csharplang copied to clipboard

Champion: Name lookup for simple name when target type known

Open gafter opened this issue 6 years ago • 61 comments

There are three contexts in which lookup of a simple (not qualified) name would benefit from the fallback of looking up the identifier in a target type.

Simple ID expression

When an expression is an unqualified identifier, and lookup fails to find any accessible binding for that identifier, it is treated as target-typed. When the target type becomes available, the identifier is looked up in the context of the target type. If the identifier in that type designates an accessible constant, static field, or static property whose type is that target type, the identifier binds to that member.

Example:

enum Color { Red, Greed }

Color M1() => Red; // OK, binds to Color.Red

int IsRed(Color c) => c switch {
    Red => true, // OK, binds to Color.Red
    _ => false,
};

Object creation expression

When an object creation expression's type is given as an unqualified identifier (possibly with type arguments), and lookup fails to find any accessible binding for that identifier, it is treated as target-typed. When the target type becomes available, the identifier is looked up in the context of the target type (and the rest of the creation expression is bound, like the target-typed new expression). If the identifier in that type designates an accessible type that has an implicit reference conversion to that target type, the identifier binds to that type.

Example:

class Outer
{
    public class Inner1: Outer { }
    public class Inner2<T>: Outer { }
}

Outer M1() => new Inner1(); // binds to Outer.Inner1
Outer M2() => new Inner2<int>(); // binds to Outer.Inner2<T>

Type in a pattern

When the type appearing in a pattern is given as an unqualified identifier (possibly with type arguments), and lookup fails to find any accessible binding for that identifier, the identifier is looked up in the context of the pattern's input type. If the identifier in that type designates an accessible type, the identifier binds to that accessible type.

Example:

class Outer
{
    public class Inner1: Outer { }
    public class Inner2<T>: Outer { }
}

int M1(Outer o) => o switch
{
    Inner1 => 1,          // ok, binds to Outer.Inner1
    Inner2<int> => 2, // ok, binds to Outer.Inner2<T>
    _ => 3,
};
bool M2(Outer o) => o is Inner1;  // ok, binds to Outer.Inner1
bool M3(Outer o) => o is Inner2<int>;  // ok, binds to Outer.Inner2<T>

Design Meetings

https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-09-26.md#discriminated-unions

gafter avatar Nov 02 '19 00:11 gafter

/cc @agocke @MadsTorgersen

gafter avatar Nov 02 '19 00:11 gafter

So with this, would I be able to do something akin to GetMethod("Name", Public | Instance) without using static System.Reflection.BindingFlags;?

DaZombieKiller avatar Nov 02 '19 01:11 DaZombieKiller

Quite useful, but what about the compiler overhead of overload resolution?

enum E1 { EA, EB, EC }
enum E2 { EA, EB }

void M(E1 e, int x);
void M(E2 e, string x);

M(EA, someParameter);

Does this meet the similar overload resolution algorithm when using number literals?

huoyaoyuan avatar Nov 02 '19 10:11 huoyaoyuan

Greed is the color of money.

mattwar avatar Nov 11 '19 19:11 mattwar

Public | Instance

As per current proposal, I think you would need to qualify at least one operand,

GetMethod("Name", BindingFlags.Public | Instance)

In order to omit type for all of them, the operator | itself needs to get target-typed.

alrz avatar Nov 12 '19 12:11 alrz

That's exactly correct, and we actually brought up the | operator in the meeting when talking about this.

333fred avatar Nov 12 '19 16:11 333fred

reflectronic avatar Nov 12 '19 18:11 reflectronic

I've encountered with an issue with using static,

case IBinaryOperation { OperatorKind: Equals } op:

Equals binds to the class member and produces an error.

For target-typed lookup, would it make sense to alter the spec as follow?

When an expression is an unqualified identifier, and lookup fails to find any valid accessible binding for that identifier, it is treated as target-typed.

The lookup itself would have no information about the context and we'll need to add a new bottleneck to check the validity which is defined as being a constant of a particular type in this case.

alrz avatar Dec 05 '19 10:12 alrz

@alrz We would need to define a concept of validity for every context in which an expression could appear. I don't think it is worth it.

gafter avatar Dec 05 '19 16:12 gafter

We would need to define a concept of validity for every context in which an expression could appear.

To avoid that, as a simpler rule, we could say if an identifier is already found but the conversion fails, we treat as target-typed. In other words, it's defined as the fallback conversion iif the source expression is a simple identifier. Then it wouldn't depend on the context.

In the example above, method group fails to convert to enum, so it will be treated as target-typed.

edit: I realized the above might be a breaking change in some scenarios involving overload resolution, candidates that were not applicable becomes applicable and could change program's behaviour.

alrz avatar Dec 06 '19 05:12 alrz

To avoid that, as a simpler rule, we could say if an identifier is already found but the conversion fails, we treat as target-typed. In other words, it's defined as the fallback conversion iif the source expression is a simple identifier. Then it wouldn't depend on the context.

That would be a breaking change. In an expression such as M(Name, e2, e3), where M is overloaded, there might be some M in the method group whose first argument would not accept the "normal" meaning of Name. But with this new rule that method would become a candidate, and that could change the result of overload resolution. I don't think we should do that.

gafter avatar Feb 27 '20 02:02 gafter

In a design note there was a consideration to separate out overload resolution in target-typing scenarios to be able to tweak conversions without breaking change. I think this one would be another case where that could help.

https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-10-28.md

I think the situation is almost the same as short x = b ? 1 : 2; for target-typed ?: expressions.

alrz avatar Feb 27 '20 07:02 alrz

I think the situation is almost the same as short x = b ? 1 : 2; for target-typed ?: expressions.

No, this is give a meaning to code that was definitely an error before. There is no breaking change in that case.

gafter avatar Jul 27 '20 23:07 gafter

No, this is give a meaning to code that was definitely an error before. There is no breaking change in that case.

I was responding to:

That would be a breaking change. In an expression such as M(Name, e2, e3)

Target-typing Name would be breaking as much as target-typing the ternary in M(b?1:2, e2, e3). I think whatever semantic is being used there, can be also applied to names so that the example above with Equals gets target-typed instead of an error.

alrz avatar Jul 28 '20 09:07 alrz

That would be a breaking change. In an expression such as M(Name, e2, e3)

If Name bound to something before, then it would not be target-typed. If it did not bind to something before, then this was illegal code. Either way, it isn't a breaking change.

gafter avatar Jul 28 '20 21:07 gafter

The idea of something being treated as target-typed only when the identifier is unqualified is IMO very dangerous, as the simple fact of adding a using directive (for something unrelated), or a library update adding a new type into a namespace you are already have a using directive for, can suddenly and silently change the meaning of the code. The above things already happen today, but the compiler flags them as ambiguous and makes you qualify/disambiguate - but that wouldn't be the case here. Sometimes it will fail, but at least some of the examples above could work, meaning: the code is unexpectedly and unobviously changed in ways that are almost impossible to detect during PR review.

Any "fallback" usage has this problem.

Some of the examples look fairly resilient to this, agreed. But not all. Consider the following, inspired by the examples in the top post:

class Outer
{
    public interface IFoo {}
}
bool M4(Outer o) => o is IFoo; 

The intent here is to to test for Outer.IFoo via target typing. Then someone adds IFoo in a <PackageReference> update that brings IFoo into scope (via a pre-existing using). So IFoo is no longer unqualified, and it is no longer target typed. The meaning is changed, and you won't be told.

I wonder whether something as simple as saying "leading period means target typed" is a pragmatic solution here:

bool M5(Outer o) => o is .IFoo; 

mgravell avatar Aug 11 '20 06:08 mgravell

@mgravell This is something that we had discussed. I apologize that the discussion did not take place here. The expression form (as opposed to the type form) is less susceptible to this issue, so we might consider target-typing only the expression form.

Unfortunately, a straightforward application of the language tactic you suggest doesn't work, as it introduces an ambiguity. There are two different syntactically valid parses of the following:

_ = A ? . B ? . C : D;

(one of the ? is part of a ?: and one is part of a ?. but there is no way to tell which is which)

We could probably make a grammar that recognizes that . identifier can never appear in a receiver position (i.e. on the left of a ?. or . operator) but that might be pretty complex and the parser might require some lookahead. I have not tried to work out the details.

gafter avatar Aug 11 '20 16:08 gafter

The expression form (as opposed to the type form) is less susceptible to this issue, so we might consider target-typing only the expression form.

Because we're target-typing only as the last resort after any other binding rule has failed, the issue may be still pretty much present. Especially if we don't do anything about https://github.com/dotnet/csharplang/issues/2926#issuecomment-562065567. You may introduce any identical name in the scope in any possible way and all the target-typed identifiers will break. I think having a prefix could help to ensure that an identifier is target-typed regardless of anything else. I think ::ident would be a good alternative since it is being used for a similar purpose in C++ (skips to global scope rather than the target-type, but we already have a syntax for that global::.)

alrz avatar Aug 12 '20 07:08 alrz

In my opinion a problem of this sort arising from this feature would be rare and if it happens would probably bring in a value of the wrong type. You would not give a variable of type Color the name Red when there is a constant Color.Red with a different value.

gafter avatar Aug 13 '20 05:08 gafter

I guess Flags enums could have this implicit conversion:

E e = { Flag1, Flag2 };

ghost avatar Sep 02 '20 18:09 ghost

Would this be allowed with object?

i.e.

enum Color { Red, Greed }

object M1() => Red; // Would this compile?

Eli-Black-Work avatar Sep 16 '20 09:09 Eli-Black-Work

No, because the target type is known to be object, not Color.

PathogenDavid avatar Sep 16 '20 09:09 PathogenDavid

Would it be feasible for Intellisense for this feature to list only the relevant values for the target type?

E.g.

enum Visibility { Visible, Hidden }

class View
{
    public Visibility Visibility { get; set; }
}

View Markup() => new View { Visibility = /* Intellisense should list only Visible and Hidden here after typing the = */ };

If that is not feasible, would using the Swift style . prefix similar to what is proposed in #764 help? Listing only the relevant target type members seems like an intuitive Intelllisense behavior after typing Visibility = ., but less so after typing just Visibility =

Without the . you would expect many more in-scope members to be listed. Using the . would let you express that you only want to see members of the target type. Focusing Intellisense like this would reduce noise while editing.

Edit: the . would decrease a lot of noise while editing but add a bit of noise while reading. Considering that time spent reading vs writing is in the order of 9:1 a proper balance would be needed. Perhaps the . could be optional, much like the this. prefix to access members in the same class is optional. You could then explicitly express limiting the scope to the target type in both reading and writing, according to your preference in the specific coding context

VincentH-Net avatar Oct 13 '20 07:10 VincentH-Net

@VincentH-Net That's more of a tooling question, and not entirely relevant when the feature isn't even worked on yet. While IntelliSense behavior can be taken into account with features, the language spec doesn't actually prescribe how such tools are required to operate. 🚃 🐴

Joe4evr avatar Oct 13 '20 08:10 Joe4evr

@Joe4evr understood; I put this in now so that this may be taken into account before too much work has been done on the feature syntax.

From the developer perspective the value of a language feature is in reading and writing combined; even though there is no prescription in the language spec for tooling, reasoning about what tooling behavior a specific syntax enables versus prevents during language design can make the feature more valuable when delivered.

I hope this can be taken into account at the appropriate moment; thanks!

VincentH-Net avatar Oct 13 '20 08:10 VincentH-Net

VincentH-Net That's more of a tooling question, and not entirely relevant when the feature isn't even worked on yet. While IntelliSense behavior can be taken into account with features, the language spec doesn't actually prescribe how such tools are required to operate. 🚃 🐴

Given that this feature is all about making it easier for developers, consideration of the experience is 100% relevant.

As to your question, @VincentH-Net, it almost certainly couldn't. You may be using a raw enum value, or you may be using a value cached in some field on some nested object. We wouldn't want to not show other items in there.

333fred avatar Oct 13 '20 15:10 333fred

We woudl likely handle this like how we handle other enums. We would show all available symbols in scope, but we would preselect the enum types/members as the likely item you'd want to insert.

CyrusNajmabadi avatar Oct 13 '20 15:10 CyrusNajmabadi

So... When this will be implemented? 😄

Jonatthu avatar Feb 15 '21 17:02 Jonatthu

This would be wonderful for situations where one has an outer class with generics and inner classes that rely on those type parameters. E.g.

public void M(Result<Exception, string> result)
{
    switch (result)
    {
          case Result<Exception, string>.Ok(var x) => ...
          case Result<Exception, string>.Error(var err) => ...
    }
}

Can become

public void M(Result<Exception, string> result)
{
    switch (result)
    {
          case Ok(var x) => ...
          case Error(var err) => ...
    }
}

Which is so much better

Richiban avatar Nov 09 '21 21:11 Richiban

I randomly found myself thinking about this issue and observed that the nature of the ambiguity in https://github.com/dotnet/csharplang/issues/2926#issuecomment-672074053 is similar to an ambiguity observed with collection literals (#5354).

M(x ? [a, b, c] : [d, e, f]);
M(x ? .a() : .b());

In both cases, it seems better to look ahead for a : and parse as a conditional expression, than to e.g. parse as a conditional access, give an error on the :, and say "parenthesize if you really want this."

RikkiGibson avatar Apr 14 '22 21:04 RikkiGibson