-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Remove indexer filtering from lookup #69947
Conversation
72065ff
to
e745ba9
Compare
@AlekseyTs @dotnet/roslyn-compiler for reviews, thanks |
@jjonescz I am having a hard time linking the fix and the issue. The issue is about an unexpected diagnostics, the unit-test doesn't even check diagnostic. SemanticModel is not supposed to affect any diagnostic reported to a user. Could you please elaborate? |
The unexpected diagnostic is because the indexer isn't found - and it's not found because it's filtered out. The test checks |
It is still not clear to me what component looks for an indexer, when, how it results in a diagnostics, etc. Could you provide a scenario with the diagnostics? |
Following #69663 (comment) reproduces the problem clearly and you can see the stack trace of the component - it's the IDE asking the compilation about the type info - similarly what I do in the unit test. I imagine the IDE then reports all diagnostics it gathers in design-time. But anyway, even without the IDE repro, the unit test I provide is still broken without the fix and it's something that should work, right? Call stack:
|
Probably. However, I see nothing that would guarantee that the problem reported by the user (the unexpected diagnostics) is fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM. One small testing comment.
var model = comp.GetSemanticModel(tree); | ||
model.GetDiagnostics().Verify(); | ||
var info = model.GetSymbolInfo(indexer); | ||
Assert.NotNull(info.Symbol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than NotNull
, I'd prefer if we did AssertEx.Equal("...", info.Symbol.ToTestDisplayString())
, so we know exactly what is getting returned here.
Fixes #69663.