Don't include snippets of source in diagnostics about types #7515
Conversation
gafter
added
Area-Compilers
4 - In Review
| internal override string ErrorDisplayName() | ||
| { | ||
| var pb = PooledStringBuilder.GetInstance(); | ||
| pb.Builder.Append(Identifier.ValueText).Append("<").Append(new string(',', Arity - 1)).Append(">"); |
There was a problem hiding this comment.
StringBuilder has Append(char, int) overload.
There was a problem hiding this comment.
Why is this the error display name? It seems appropriate for an unbound generic. However, if this is a bound generic, why not have the type argument list?
There was a problem hiding this comment.
Because when we are unable to resolve the type name, the actual type arguments are irrelevant.
|
LGTM |
| var pb = PooledStringBuilder.GetInstance(); | ||
| pb.Builder.Append(Identifier.ValueText).Append("<").Append(new string(',', Arity - 1)).Append(">"); | ||
| return pb.ToStringAndFree(); | ||
| } |
There was a problem hiding this comment.
Yes, some of the changed tests are due to this.
|
These changes allows for the following to compile incorrectly This is slightly modified version of the failing code action test case. I think the error should still be CS0246. Update: Possible fix looks easy, just verifying the previous failing case. |
|
This is failing due to an incorrect check now for to Also consider adding the following (and more if there are) as a test: to report CS0246 Note: Not sure how expensive |
|
This PR also seems to partially fix #7493 to bring it inline with what the legacy compiler reported. |
| @@ -1524,7 +1524,7 @@ public int Compare(Symbol fst, Symbol snd) | |||
| node = node.Parent; | |||
| } | |||
|
|
|||
| CSDiagnosticInfo info = NotFound(where, simpleName, arity, where.ToString(), diagnostics, aliasOpt, qualifierOpt, options); | |||
| CSDiagnosticInfo info = NotFound(where, simpleName, arity, simpleName, diagnostics, aliasOpt, qualifierOpt, options); | |||
There was a problem hiding this comment.
It looks like there are only two call sites for this function and now both are passing the same value for the second and the forth parameter. We should probably get rid of one of them to simplify things.
There was a problem hiding this comment.
Not so. One of the call sites looks like this:
NotFound(where, simpleName, arity, whereText + "Attribute", diagnostics, aliasOpt, qualifierOpt, options | LookupOptions.VerbatimNameAttributeTypeOnly);
|
@dotnet/roslyn-compiler Could I have a second review, please? |
| @@ -1524,7 +1524,7 @@ public int Compare(Symbol fst, Symbol snd) | |||
| node = node.Parent; | |||
| } | |||
|
|
|||
| CSDiagnosticInfo info = NotFound(where, simpleName, arity, where.ToString(), diagnostics, aliasOpt, qualifierOpt, options); | |||
| CSDiagnosticInfo info = NotFound(where, simpleName, arity, (where as NameSyntax)?.ErrorDisplayName() ?? simpleName, diagnostics, aliasOpt, qualifierOpt, options); | |||
| return new ExtendedErrorTypeSymbol(qualifierOpt ?? Compilation.Assembly.GlobalNamespace, simpleName, arity, info); | |||
There was a problem hiding this comment.
It looks like lines of code 1285, 1424, 1431 are also using syntax node as a diagnostics argument. Should they be changed too?
There was a problem hiding this comment.
@AlekseyTs Those are fixed now, too. Can you please re-review?
|
@dotnet-bot test this please |
| @@ -1424,6 +1424,17 @@ static int Main(string[] args) | |||
| Assert.Equal(CandidateReason.OverloadResolutionFailure, symbolInfo.CandidateReason); | |||
| } | |||
|
|
|||
| [Fact] | |||
There was a problem hiding this comment.
Should this test be linked to the issue?
|
It looks like the baseline for Microsoft.CodeAnalysis.CSharp.UnitTests.CompilationErrorTests.CS0104ERR_AmbigContext01 test should be adjusted. |
|
LGTM, modulo the base line adjustment for CS0104ERR_AmbigContext01. |
Fixes #7387
@dotnet/roslyn-compiler Please review.