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
Fixes rude error reporting for lifted implicit value parameters #14276
Conversation
@CyrusNajmabadi @dotnet/roslyn-compiler @dotnet/roslyn-interactive |
// Note that in VB implicit value parameter in property setter doesn't have a location. | ||
// In C# its location is the location of the setter. | ||
// See https://github.com/dotnet/roslyn/issues/14273 | ||
return local.Locations.FirstOrDefault()?.SourceSpan ?? local.ContainingSymbol.Locations.First().SourceSpan; |
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.
This is in AbstractEditAndContinueAnalyzer. So why not have two subclasses of this (for VB and C#) and have hte language specific cases handled there?
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.
We do have two subclasses, but I prefer not to specialize if not necessary.
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.
Once the bug is fixed we can remove the null conditionals.
test windows_release_unit64_prtest please |
if (method != null) | ||
{ | ||
if (method.MethodKind == MethodKind.EventAdd || | ||
method.MethodKind == MethodKind.EventRemove || | ||
method.MethodKind == MethodKind.PropertySet) | ||
{ | ||
return symbol.Name == "value"; | ||
// the name is value in C#, and Value in VB | ||
return symbolOpt.Name == "value" || symbolOpt.Name == "Value"; |
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.
Just to verify, but this should never make a parameter in c# called "Value" get considered as an IsImplicitValueParameter right?
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.
Nope, since it has to be implicitly declared and the compiler doesn't produce such parameter.
👍 |
Fixes VS crash - internal bug 234448