Warn on type named "record" #47094
Warn on type named "record" #47094
Conversation
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
Outdated
Show resolved
Hide resolved
|
Done review pass (commit 1) #Closed |
| var comp = CreateCompilation(src); | ||
| comp.VerifyDiagnostics( | ||
| // (2,9): warning CS8860: Types and aliases should not be named 'record'. | ||
| // class C<@record> { } |
There was a problem hiding this comment.
This feels odd to me, if the user has escaped an identifier presumably they are aware of its similarity to a language keyword and do not wish to be bothered about it. #Closed
| // (2,1): hidden CS8019: Unnecessary using directive. | ||
| // using @record = System.Console; | ||
| Diagnostic(ErrorCode.HDN_UnusedUsingDirective, "using @record = System.Console;").WithLocation(2, 1), | ||
| // (2,7): warning CS8860: Types and aliases should not be named 'record'. |
There was a problem hiding this comment.
This seems incorrect. For example, using @string = System.Console is perfectly legal. #Closed
|
Done review pass (commit 2)
I would disagree about the blockingness of the comment :) #Closed |
| modifierErrors = true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (this.Name == SyntaxFacts.GetText(SyntaxKind.RecordKeyword)) | ||
| { | ||
| var location = declaration.Declarations[0].NameLocation; |
There was a problem hiding this comment.
Would this not issue an error if you had a partial declaration where the "first" declaration was escaped, but the second one wasn't? Consider testing something like:
partial class @record {}
partial class record {}
``` #ResolvedThere was a problem hiding this comment.
Actually, I'm not even sure that this code is necessary? It seems that the only direct implementations of this type are SourceNamedTypeSymbol (already modified in this PR), ImplicitNamedTypeSymbol (which does not have a user-controlled name), and SimpleProgramNamedTypeSymbol (also doesn't have a user-controlled name). Do we have a test case that fails if this is removed?
In reply to: 477450215 [](ancestors = 477450215)
There was a problem hiding this comment.
Many tests fail without this logic. This is responsible for checking the name of the type. The logic added in SourceNamedTypeSymbol is responsible for checking type parameters.
For instance this test fails:
public void TypeNamedRecord()
{
var src = @"
class record { }
class C
{
record M(record r) => r;
}";
var comp = CreateCompilation(src);
...
In reply to: 477453969 [](ancestors = 477453969,477450215)
|
Done review pass (commit 4) #Closed |
| } | ||
|
|
||
| [Fact, WorkItem(47090, "https://github.com/dotnet/roslyn/issues/47090")] | ||
| public void TypeNamedRecord_BothEscapedPartial() |
There was a problem hiding this comment.
Nit: this is not both escaped :) #Pending
| @@ -151,6 +151,8 @@ internal string GetDebuggerDisplay() | |||
| diagnostics.Add(ErrorCode.ERR_NoAliasHere, usingDirective.Alias.Name.Location); | |||
| } | |||
|
|
|||
| SourceMemberContainerTypeSymbol.ReportTypeNamedRecord(usingDirective.Alias.Name.Identifier.Text, compilation, diagnostics, usingDirective.Alias.Name.Location); | |||
There was a problem hiding this comment.
usingDirective.Alias.Name [](start = 78, length = 25)
Consider extracting a local. #Resolved
| foreach (var decl in declaration.Declarations) | ||
| { | ||
| var location = decl.NameLocation; | ||
| var nameText = location.SourceTree.GetRoot().FindToken(location.SourceSpan.Start).ToString(); |
There was a problem hiding this comment.
location.SourceTree.GetRoot().FindToken(location.SourceSpan.Start).ToString(); [](start = 35, length = 78)
This seems too heavyweight. Could we use direct access instead? Perhaps ((decl as SingleTypeDeclaration)?.SyntaxReference.GetSyntax() as BaseTypeDeclarationSyntax)?.Identifier.ValueText. #Pending
There was a problem hiding this comment.
Or change the loop to loop over SyntaxReferences:
foreach (var syntaxRef in SyntaxReferences)
{
var location = syntaxRef.GetLocation();
var nameText = (syntaxRef as BaseTypeDeclarationSyntax)?.Identifier.ValueText;
...
}
In reply to: 478653015 [](ancestors = 478653015)
ghost
added this to the 
* upstream/master: (43 commits) Fix typos (dotnet#47264) Disable CS0660 where Full Solution Analysis produces a different result Pass in correct arguments to TypeScript handler Add skipped failing test for IDE0044 (dotnet#45288) Avoid first chance exception Code review feedback part 3 Update stale URLs in the readme Update IntegrationTestSourceGenerator.cs Fix comment Update IntegrationTestSourceGenerator.cs Remove unused reference to obsolete class Update nullable annotations in Classification folder Update generator public API names: (dotnet#47222) NullableContextAttribute for property parameters is from accessor (dotnet#47223) Modify global analyzer config precedence: (dotnet#45871) Skip the C# source generator integration test running for now Additional XAML LSP handlers (dotnet#47217) Remap diagnostics using IWorkspaceVenusSpanMappingService in-proc Warn on type named "record" (dotnet#47094) Bail out early before getting SyntaxTree ...
Fixes #47090