Roslyn: UnboundIdentifiers: 'nameof' does not exist in the current context.

[vsfeedback]

In a lambda expression. If I write a #warning directive, the nameof will be error with

CS0103 The name 'nameof' does not exist in the current context.

Test codes:

Action action = () =>
{
    var x = nameof(System);
#warning xxx
};

But there is an interesting phenomenon, the codes can be compile successed and run correctly.

In my test, this bug is appears in both of Visual Studio 2015 and Visual Studio 2017.

_This issue has been moved from https://developercommunity.visualstudio.com/content/problem/71314/nameof-does-not-exist-in-the-current-context.html
VSTS ticketId: 454314_
_These are the original issue comments:_

彭闽 on 6/21/2017, 11:43 PM (169 days ago):

Action action = () => {

    var x = nameof(System);


warning xxx

};

Dan J on 8/30/2017, 11:06 AM (99 days ago): I also have this happen in 15.3.3, does not prevent build from suceeding, but constantly having 20 errors makes it hard to find what is actually boken
_These are the original issue solutions:_
(no solutions)

[PMExtra]

Test Code:
```C#
Action action = () => {
var x = nameof(System);

warning xxx

};
```

[jcouv]

@heejaechang @mavasani
I can see that this diagnostic (ERR_NameNotInContext) is produced when binding the identifier nameof by itself (as opposed to the invocation nameof(System)) when the IDE call the semantic model on it (GetSymbolInfo on the identifier). But the semantic model logic properly discards that diagnostic.

Any idea where or how this diagnostic might surface to the IDE?
One thing I noticed is that this diagnostic only appears in the IDE if there is another (real) error in the code (such as the #warning or some other bad code).

Note this also repros in sharplab.

```C#
[Fact]
[WorkItem(23667, "https://github.com/dotnet/roslyn/issues/23667")]
public void NameofNotFound()
{

        var source = @"

class Program
{
public static void Main()
{
System.Action action = () => {
var x = nameof(System);
System.Console.Write(x);

warning xxx

    };
}

}";
var comp = CreateStandardCompilation(source);

        var tree = comp.SyntaxTrees.Single();
        var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
        var nameof = tree.GetRoot().DescendantNodes().OfType<InvocationExpressionSyntax>().ElementAt(0).Expression;

        var symbol = model.GetSymbolInfo(nameof);

// stepping through this code I can see that the diagnostic is produced, but it gets discarded (not sure how it reaches the IDE)
}
```

[heejaechang]

@jcouv do you have some external plug in installed? we won't show blue squiggle by default. or do you have build error as blue squiggle internal option on? that is off by default, and hidden, you need to explicitly edit reg key to turn it on, did you do that?

[heejaechang]

@jcouv you can set break points here (all analyzeXXX methods)
http://source.roslyn.io/#Microsoft.CodeAnalysis/DiagnosticAnalyzer/CompilerDiagnosticAnalyzer.CompilationAnalyzer.cs,30

and then open the file that shows the error and check the diagnostics returned by those method. one of them should contain that error if it surface to IDE as long as it is us who report it not third party analyzers.

[jcouv]

I think the blue squiggles are a red herring. They do not appear on my machine, where I'm still able to repro the problem.

Thanks, I'll try debugging the method you mentioned.

[jcouv]

@heejaechang Thanks for the tip.
Unfortunately, I was not able to intercept this squiggle through CompilerDiagnosticAnalyzer. When I set breakpoints in the various Analyze methods and also log calls to the ReportDiagnostics method, I only see the other two diagnostics (shown as green squiggles), but not the error that produces red squiggles on nameof (ERR_NameNotInContext).

Let me know if you could take a look at it with me. I have a repro set up. Thanks

[heejaechang]

@jcouv let me take a look. I will get back to you tonight or tomorrow.

[heejaechang]

@jcouv the error is reported by "Microsoft.CodeAnalysis.CSharp.Diagnostics.CSharpUnboundIdentifiersDiagnosticAnalyzer" (http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp.Features/Diagnostics/Analyzers/CSharpUnboundIdentifiersDiagnosticAnalyzer.cs,23)

[jcouv]

Thanks a bunch @heejaechang .

I thought I had searched for "CS0103" in the entire codebase (because I remembered another instance of a similar confusion). I must have messed up :-(

Analyzers that produce compiler error codes are evil. What do you think of requiring analyzers to produce their own error codes?
I'll change this analyzer and try to put a guard against this happening again.

[heejaechang]

tagging @jinujoseph @Pilchie for "What do you think of requiring analyzers to produce their own error codes?"

[jinujoseph]

cc @mavasani did not you had a some thing similar in analyzer repo

[Pilchie]

I think that's what we generally expect and desire, but I'm not sure how well we can enforce it. At best we can enforce that a Workspace doesn't have multiple providers that produce the same error code.

On the other hand, one could imagine an author deliberating separating different reporting of the same "error" into multiple providers (a fast one and slow one, etc).

[heejaechang]

I think for this particular case, IDE intentionally produced diagnostic with compiler error code to handle some corner error case I believe. question is, can we just use IDE own error code and change fixer to look for that error as well? rather than pretend error is from compiler?
...

by the way, currently, what "CSharpUnboundIdentifiersDiagnosticAnalyzer" does is supported scenario. we even designed for it (diagnostic analyzer and fixer being loosely coupled). but in real life, if a diagnostic analyzer produces a diagnostic at random location with an error code existing fixer claims they can handle, most likely, the fixer will fail (throw exception). so this loosely coupled thing doesn't work as much as we hoped for. so reasoning to share same error code is not as good as we thought it would be.

[mavasani]

I think following are guidelines:

1. Different analyzers in same compilation shouldn't use duplicate IDs - that is very like a bug.
2. Different analyzers in different compilation may use the same ID - this may be a bug, but could also very likely be a case where different analyzers report similar issues for different constructs OR have different implementation of similar diagnostic. We can probably consider reporting a host diagnostic when we see different analyzers in different packages reporting same diagnostic.
3. Analyzers reporting CS or BC diagnostics - this is normally done to augment compiler diagnostics where analyzer feels compiler is missing reporting some cases and the author wants FixAll to fix all the compiler and augmented diagnostics. @ivanbasov faced a similar issue where we compiler was not reporting unused local diagnostics for local functions. We can consider prohibiting this by writing an analyzer in Microsoft.CodeAnalysis.Analyzers that flags any analyzer reporting diagnostic with CS or BC prefix. User will have to perform two FixAll operations to fix compiler and analyzer diagnostics separately, but that seems acceptable to me.

[mavasani]

We also recently added an analyzer diagnostic in Microsoft.CodeAnalysis.Analyzers for case 1. above: https://github.com/dotnet/roslyn-analyzers/blob/26fe065161a871718c717da1821d82c4decbc8e9/src/Microsoft.CodeAnalysis.Analyzers/Core/CodeAnalysisDiagnosticsResources.resx#L296

```

Diagnostic Id '{0}' is already used by analyzer '{1}'. Please use a different diagnostic ID.

[heejaechang]

@mavasani hmmm this (https://github.com/dotnet/roslyn/issues/23667#issuecomment-351474275) is not actually the way we originally designed it for. and coded for.

probably in real life, what you wrote it true though. so, I think we need to make up our mind on this issue

tagging @jinujoseph @Pilchie @dotnet/roslyn-analysis

[heejaechang]

"We can consider prohibiting this by writing an analyzer in Microsoft.CodeAnalysis.Analyzers that flags any analyzer reporting diagnostic with CS or BC prefix. User will have to perform two FixAll operations to fix compiler and analyzer diagnostics separately, but that seems acceptable to me."

this is interesting. for fix all, do we only care about equivalent key or (error code + equivalent key)?
..
just looked code. fix all can handle multiple diagnostics ID at the same time as long as it can be fixed by 1 fixer and code action returns same equivalent key. so, I think fix all will work as well.

[jcouv]

For preventing analyzers from producing "CS..." diagnostics, I was thinking of adding a debug assertion in the `AnalyzerExecutor.IsSupportedDiagnostic". It already has logic to distinguish the compiler analyzer from the rest.

[mavasani]

For preventing analyzers from producing "CS..." diagnostics, I was thinking of adding a debug assertion in the `AnalyzerExecutor.IsSupportedDiagnostic".

Nice idea @jcouv! I would recommend going a step further and instead of adding a debug assert, we throw an ArgumentException with invalid diagnostic ID, just like we do for supported diagnostics with invalid identifier as an ID (see here). We probably want a slightly different message then CodeAnalysisResources.InvalidDiagnosticIdReported

[CyrusNajmabadi]

Ideally CSharpUnboundIdentifiersDiagnosticAnalyzer wouldn't even exist. It's a hacky workaround to deal with the issue of the compiler not reporting errors in some cases. I tried to remove it a while back, but ran into several scenarios (i think around lambdas) where we wouldn't get appropriate compiler errors.

[CyrusNajmabadi]

I think a better solution, in the future, woudl be to have the compiler emit these diagnostics, but convey that they dont' need to be shown by default in a UI (like an error list, console, or squiggle). They would still convey to fixers that the issue is there, but they wouldn't cause cascading error-hell in the UI.

[mavasani]

I think a better solution, in the future, woudl be to have the compiler emit these diagnostics, but convey that they dont' need to be shown by default in a UI

I think compiler doesn't even bind method bodies when there are top level semantic errors. Perhaps the GetDiagnostics API can be parameterized to control this behavior differently for IDE and batch compilation scenarios.

[CyrusNajmabadi]

I think compiler doesn't even bind method bodies when there are top level semantic errors.

That's definitely not true. If it didn't bind method bodies if there was a top level semantic error the entire IDE experience woudl fall over. You can observe this here:

[CyrusNajmabadi]

batch compilation scenarios.

Yes. For batch compilation, the compiler doesn't bother going into methods. But that's not really important when it comes to the IDE fixer scenario. :)

[jcouv]

With my change (https://github.com/dotnet/roslyn/pull/23776), a distinct diagnostic is now produced (shown below).

I took a look at the UnboundIdentifier analyzer to avoid it complaining on nameof, but I couldn't find how to fix it. I'll send this one over to the IDE team for further investigation.

If we can't find a good solution, I'd be tempted to remove this analyzer, even if it means losing some opportunities for the GenerateType/GenerateConstructor fixers.

[jcouv]

@jinujoseph I'd like to hand this analyzer issue to IDE team. I'll let you triage. Thanks

[heejaechang]

I think what @mavasani meant to say is when lambda has an issue, compiler doesn't even bother to bind inside of lambda method body. also when top level member is incomplete, compiler doesn't bother to bind those member even if it is specifically asked to bind those (IDE case).

if compiler doesn't do that, I think we can remove this unbound identifier analyzer. if they can't, I think we should at least change error code.

[LeeorV]

I ran into this issue yesterday, in a real scenario in which I wanted to check which property changed in a "PropertyChanged" event, and act accordingly.

after tinkering around a bit, the error changed to "Expression cannot be used in an argument to nameof".

Can anyone here explain what this error means, and why this is the case?

this is the faulty code:

when I changed to a using pattern matching to produce a temp local, it fixed the problem.

Can anyone explain why the 2nd option is valid, but the first is not?

[jcouv]

@LeeorV nameof takes an identifier of a program entity and produces a string.
It does not apply to general expressions, such as i_Sender as TileDataModel. It's not clear to me what that would even mean or do, or why it would return something with a TitleItem member.

[LeeorV]

i_Sender is an Object type reference to the object that invoked the "PropertyChanged" event. in order to have access to that object's members/properties, I then cast that Object reference to the sender's actual type - a custom class named "TileDataModel" which happens to have a property named "TileItem".

Basically, I want to know if the property that was changed (which invoked the event) is the property named "TileItem", but to avoid embedding its name as a string, I use nameof instead.

As shown in the 2nd picture, when I have a local reference variable of type "TileDataModel" (created via pattern matching and named tdm), nameof works correctly.

Why doesn't it do the same to the casting result, which is statically known to be of type "TileDataModel" at compile time (whether there is actually a reference there, or just a Null, is not known at compile time, but the reference type is known).?

Changing nameof((i_Sender as TIleDataModel).TileItem) to nameof(((TileDataModel)i_Sender).TileItem) didn't work either, so it doesn't matter if the casting is dynamic or static.

[jcouv]

I'd misread the portion about TitleItem member.
Still, nameof doesn't accept general expressions. Just do nameof(TileDataModel.TitleItem).

[LeeorV]

I was actually not aware that this was possible with non-static properties.

This makes a lot of sense, and is much more readable, and I will do this in the future. Thanks!

[mavasani]

@dotnet/roslyn-compiler @dotnet/roslyn-ide @CyrusNajmabadi Does anyone know the history of UnboundIdentifiersDiagnosticAnalyzer that tries to mimic the compiler behavior for unknown identifiers by walking errorneous code with lambda and incomplete members to generate such a diagnostic, so that we can offer an add-using code fix for such cases? Do we know why the compiler doesn't produce CS0103 for such cases? If it did, when we can get rid of this analyzer.

I am going to move this bug over to the compiler team to triage reporting these diagnostics. Note that this will not be a breaking change as the scenario we are talking about involves lambda with errors and incomplete members.

[heejaechang]

@mavasani I don't know the reason. @gafter or @jaredpar might know why compiler don't produce those diagnostics?

but, I believe we (IDE) team opened a bug to compiler about this issue when we added the unboundidentifiersDiagnosticAnalyzer first time. not sure what happened to that bug.

[jcouv]

@mavasani The present bug is about the analyzer producing a diagnostic when it should not. So I don't understand why you suggest the compiler should produce a diagnostic where it currently doesn't.

In my opinion, there are two possible ways to resolve this bug:

1. this analyzer should understand nameof and not expect the semantic model to provide a symbol for it
2. the semantic model should provide a symbol for nameof

The trouble with (2) is that I don't know what symbol could be returned. So I favor approach (1), which is an analyzer fix.

Update: After further reflection, I think you're proposing a third approach, which would be removing the analyzer entirely (and having the compiler produce diagnostics). Is that correct?

[CyrusNajmabadi]

Does anyone know the history of UnboundIdentifiersDiagnosticAnalyzer that tries to mimic the compiler behavior for unknown identifiers by walking errorneous code with lambda and incomplete members to generate such a diagnostic, so that we can offer an add-using code fix for such cases?

This history is precisely what you just described. The problem is that the compiler doesn't walk lambdas in some cases (probably cascading errors or something). As such, we don't get diagnostics. And because we've unfortunately made things so that fixes are driven by compiler diagnostics, that impacts the IDE. The solution was to make the IDE report errors in these sorts of cases. Though an alternative approach would have been to make IDE analyzer fixes driven not by compiler errors, but by specific IDE analyzers (which was my preference, but i was on TS at the time :)).

Do we know why the compiler doesn't produce CS0103 for such cases? If it did, when we can get rid of this analyzer.

This is reversed. The compiler correctly does not produce this error. The IDE analyzer tries to mimic the compiler, but gets this wrong.

--

Note: i woudl love to remove this anlayzer. It's an ugly hack.

[jasonmalinowski]

Update: After further reflection, I think you're proposing a third approach, which would be removing the analyzer entirely (and having the compiler produce diagnostics). Is that correct?

Yes, this. I understand why the compiler is doing the behavior it's doing, but it makes it a PITA for the IDE side of things when we essentially get to reinvent your diagnostics, and poorly. And it creates a whole pile of problems around the essential question of whether an analyzer should be allowed to create compiler diagnostics in the first place.

In other words, the compiler made a good decision for it's scenarios, and breaks everything else. Let's see if we can find something better?

Note: i woudl love to remove this anlayzer. It's an ugly hack.

Ditto. 👍

[mavasani]

Update: After further reflection, I think you're proposing a third approach, which would be removing the analyzer entirely (and having the compiler produce diagnostics). Is that correct?

@jcouv That is correct. I have a PR to fix this bug as per your suggested approach 1: https://github.com/dotnet/roslyn/pull/24071. @jasonmalinowski correctly suggested we should consider removing this analyzer if compiler is ready to report these additional diagnostics in the lambdas and incomplete members (approach 3)

[CyrusNajmabadi]

Note: https://github.com/dotnet/roslyn/issues/7536 tracks incomplete members. We have some issue tracking lambdas. But i'm not sure which one it is.

[mavasani]

@jcouv Is this something we plan to implement soon in the compilers? Otherwise, we should probably go ahead with the analyzer bug fix https://github.com/dotnet/roslyn/pull/24071 for now, and keep this as a tracking issue for compiler support and subsequent analyzer removal.

[AlekseyTs]

Can we clearly summarize what is being proposed here?

[mavasani]

@AlekseyTs Compiler does not produce CS0103 diagnostic for unbound identifiers within broken lambdas and incomplete members. AddUsings code fix provider operates on such compiler diagnostics, and lack of these diagnostics causes us to not offer this code fix for these cases. As a workaround, we added an IDE analyzer that tries to mimic the compiler by scanning for such cases and then reporting unbound identifier diagnostics, so the code fix is provided. We are requesting that the compiler reports these diagnostics, so we can remove this IDE analyzer, which has had lot of bugs.

[AlekseyTs]

I am not convinced that the compiler should do this.

[CyrusNajmabadi]

@gafter For his thoughts here. The issues around not binding some lambdas, and not binding incomplete members has bit the IDE numerous times.

Basically, for better or worse, the IDE has taken a dependency on compiler diagnostics to drive these features. In one sense that's good (since trying to reimplement compiler rules is really hard). But in other, it's not, since you get into situations like the one we're in now.

In my (biased) opinion, i think it makes far more sense for this sort of analysis to happen at the compiler level. It fits into the approach the roslyn compiler has taken so far where it does a reasonable job providing good semantic information over acceptably broken code cases. i.e. it's not an ask for the compiler to do a good job when gibberish is passed in. But rather that the compiler provide reasonable diagnostics when the user types normal code patterns.

--

Now, that said, there are def concerns about the compiler doing this. For example, one would be that this might cause more errors than would be liked in common cases. However, i think we could come up with a solution that addresses both needs. For example, the compiler could analyze these lambdas/incomplete members and report diagnostics for them. However, these diagnostics could be tagged as being hidden from the display (i.e. no error-list entry). The IDE could then still drive features on top of these diagnostics, without the worry of the user experience around diagnostics degrading.

IMO though, i don't think the experience would actually be degraded. I think having compiler sguiggles show up in these lambdas/incomplete members would be fine. That's the approach we took with TypeScript/JavaScript and it was never an issue in practice.

[CyrusNajmabadi]

Also, as an aside, from an implementation perspective, one thing you could consider would be removing incomplete members entirely. In TypeScript we took this approach:

1. If you wanted to make an incomplete member because you had a prefix (like attributes), but not enough to determine what sort of entity it was:
2. Determine the smallest syntactically generatable construct from all the productions of that type. (i.e. what is the smallest class member, the smallest statement).
3. Once you get that prefix, even if you don't encounter enough syntax to know what you have, assume you have that smallest syntactically generatable construct.

So, for C#, that means if you see attributes/modifiers (but nothing else), just assume you have a field declaration (because a field can be written with just two tokens Type name). You can then report some reasonable diagnostics (like 'invalid token encountered'), but you still make a FieldSyntax node in the tree (with missing nodes for the type and name). This means the FieldSyntax then flows through the rest of the binding path like normal (meaning things like attributes automatically get checked).

This may mean having to make some parts of the system handle empty/missing-names a bit better. But, in general, that's easy enough to do, and the compiler has already gone through the exercise of being resilient to that sort of thing in many other cases (i.e. class declarations with a missing name).

--

Just a thought. But this approach served us quite well in TypeScript, avoiding this entire class of issues entirely.

[heejaechang]

+1 compiler generating all diagnostics and getting rid of this from IDE side.

[BrainSlugs83]

For me, this happens (in latest VS2017 [15.6.3]) even if the #warning is just right in the outer method -- like a good 15 lines away from the lambda expression... I had no idea why it was complaining that nameof was not defined until I stumbled on to this GitHub issue.

Out of curiosity, I scrolled way down until I found the #warning I had added in the outer method and removed it, and the error went away entirely. -- It kind of sucks that this is broken, but at least my code compiles now. Thanks for posting this bug.

[gafter]

I think the compiler should report semantic diagnostics from broken lambdas just like it does in broken methods.

[SylwesterZarebski]

Error is still in 15.8.4. Is there any roadmap to fix it?

[jaredpar]

@SylwesterZarebski this bug is currently targeted at 16.0 for the earliest fix.

[SylwesterZarebski]

Thanks for information. :-)

[nnpcYvIVl]

This is still an issue in Visual Studio 2019 Preview 1.

[mavasani]

@jcouv Should we consider taking https://github.com/dotnet/roslyn/pull/24071 as a workaround until we make progress on this issue? No reason to block the user experience on implementation details, especially given this IDE analyzer already exists.

[jcouv]

Yes, it sounds like a good idea at this point.
You can link the IDE change to this issue if it needs to be undone once the compiler layer is fixed.

[jasonmalinowski]

How much work does it just take to fix the compiler layer?

[mavasani]

How much work does it just take to fix the compiler layer?

@jcouv can provide the cost. However, given that this bug was filed precisely a year ago, with multiple customer complaints since then, IMO we should take the fix to the existing IDE analyzer in Preview2 unless the compiler can fix this in Preview2.

[CyrusNajmabadi]

However, given that this bug was filed precisely a year ago, with multiple customer complaints since then,

I don't see how the age of the bug is relevant here :D. We have had several bugs over hte years that stem from the same root cause. Some of them are likely 5+ years old. We've been ok in that time accepting our current behavior, so i'd actually argue that that indicates we can accept the status quo. If we were to invest time into this, i'd rather us invest at the right level, and deal with all the issues at once, instead of continually trying to paper over the root cause with complex and wonky solutions at the IDE level :)

--

In other words, this hasn't been significant enogh to have to deal with up till now. So why did it become critical for preview 2 to have a solution now?

[CyrusNajmabadi]

Enh. I've changed my mind. If it's cheap and easy, i'm ok moving forward. I do really wish that we'd just fix the underlying compiler issue though. This is annoying and has been problematic for quite a while. And it just doesn't seem that difficult to address in a reasonable fashion.

[mavasani]

If we were to invest time into this, i'd rather us invest at the right level

The reason I was proposing taking the fix to the IDE analyzer is that we already have a PR for the fix, so there is no further investment for it from us, but with noticeable customer benefit. I agree with your concerns about the core issue slipping further down the radar with the workaround, but that does not justify continuous pain for the customers :-)

[CyrusNajmabadi]

@mavasani Sounds good to me :) A little bummed that we keep dragging this code along. But i agree that in terms of costs and outcomes, it's a very reasonable approach to take now.

[mavasani]

We discussed this at the last design meeting and it was decided that we take the fix to the analyzer for 16.0, and keep this issue open to track compiler support for correctly reporting errors within a lambda.

[jcouv]

@mavasani This issue got close automatically. It would probably be good to file a new issue for the compiler (possibly with a new repro?).

[mavasani]

Thanks @jcouv. Filed https://github.com/dotnet/roslyn/issues/32628 to track the compiler feature request and https://github.com/dotnet/roslyn/issues/32629 to follow it up deleting this IDE analyzer.