Skip to content
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.

Sign up for GitHub

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

Warn on type named "record" #47094

Merged
merged 9 commits into from
Aug 28, 2020
Merged

Warn on type named "record" #47094

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c83b766
Warn on type named record
jcouv Aug 24, 2020
ec43c41
Gate with LangVer. Block aliases and type parameters
jcouv Aug 25, 2020
4e0adf6
Allow escaped name
jcouv Aug 26, 2020
37c83d3
move local declaration
jcouv Aug 26, 2020
919f407
Check every part of partials
jcouv Aug 26, 2020
0694ca7
Fix a test
jcouv Aug 26, 2020
0ba1a06
Merge remote-tracking branch 'dotnet/master' into record-tweaks
jcouv Aug 26, 2020
7d1b65a
Address feedback from Chuck
jcouv Aug 27, 2020
5e4496b
Revert unintended change
jcouv Aug 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Imports.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ public static Imports FromSyntax(
diagnostics.Add(ErrorCode.ERR_NoAliasHere, usingDirective.Alias.Name.Location);
}

SourceMemberContainerTypeSymbol.ReportTypeNamedRecord(usingDirective.Alias.Name.Identifier.Text, compilation, diagnostics, usingDirective.Alias.Name.Location);
Copy link
Member

@cston cston Aug 27, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usingDirective.Alias.Name [](start = 78, length = 25)

Consider extracting a local. #Resolved


string identifierValueText = usingDirective.Alias.Name.Identifier.ValueText;
if (usingAliases != null && usingAliases.ContainsKey(identifierValueText))
{
Expand Down
6 changes: 6 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6416,6 +6416,12 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_CloneDisallowedInRecord" xml:space="preserve">
<value>Members named 'Clone' are disallowed in records.</value>
</data>
<data name="WRN_RecordNamedDisallowed" xml:space="preserve">
<value>Types and aliases should not be named 'record'.</value>
</data>
<data name="WRN_RecordNamedDisallowed_Title" xml:space="preserve">
<value>Types and aliases should not be named 'record'.</value>
</data>
<data name="ERR_NotOverridableAPIInRecord" xml:space="preserve">
<value>'{0}' must allow overriding because the containing record is not sealed.</value>
</data>
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1853,7 +1853,7 @@ internal enum ErrorCode
ERR_InvalidWithReceiverType = 8857,
ERR_NoSingleCloneMethod = 8858,
ERR_CloneDisallowedInRecord = 8859,
// available = 8860,
WRN_RecordNamedDisallowed = 8860,
ERR_UnexpectedArgumentList = 8861,
ERR_UnexpectedOrMissingConstructorInitializerInRecord = 8862,
ERR_MultipleRecordParameterLists = 8863,
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,7 @@ internal static int GetWarningLevel(ErrorCode code)
case ErrorCode.WRN_IsPatternAlways:
case ErrorCode.WRN_SwitchExpressionNotExhaustiveWithWhen:
case ErrorCode.WRN_SwitchExpressionNotExhaustiveForNullWithWhen:
case ErrorCode.WRN_RecordNamedDisallowed:
return 1;
default:
return 0;
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Generated/ErrorFacts.Generated.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,8 @@ private ImmutableArray<SourceMethodTypeParameterSymbol> MakeTypeParameters(Diagn
}
}

SourceMemberContainerTypeSymbol.ReportTypeNamedRecord(identifier.Text, this.DeclaringCompilation, diagnostics, location);

var tpEnclosing = ContainingSymbol.FindEnclosingTypeParameter(name);
if ((object?)tpEnclosing != null)
{
Expand Down
28 changes: 21 additions & 7 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,6 @@ private DeclarationModifiers MakeModifiers(TypeKind typeKind, DiagnosticBag diag
var mods = MakeAndCheckTypeModifiers(
defaultAccess,
allowedModifiers,
this,
diagnostics,
out modifierErrors);

Expand Down Expand Up @@ -324,7 +323,6 @@ private DeclarationModifiers MakeModifiers(TypeKind typeKind, DiagnosticBag diag
private DeclarationModifiers MakeAndCheckTypeModifiers(
DeclarationModifiers defaultAccess,
DeclarationModifiers allowedModifiers,
SourceMemberContainerTypeSymbol self,
DiagnosticBag diagnostics,
out bool modifierErrors)
{
Expand Down Expand Up @@ -356,7 +354,7 @@ private DeclarationModifiers MakeAndCheckTypeModifiers(
var info = ModifierUtils.CheckAccessibility(mods, this, isExplicitInterfaceImplementation: false);
if (info != null)
{
diagnostics.Add(info, self.Locations[0]);
diagnostics.Add(info, this.Locations[0]);
modifierErrors = true;
}
}
Expand All @@ -383,12 +381,12 @@ private DeclarationModifiers MakeAndCheckTypeModifiers(
if ((result & DeclarationModifiers.Partial) == 0)
{
// duplicate definitions
switch (self.ContainingSymbol.Kind)
switch (this.ContainingSymbol.Kind)
{
case SymbolKind.Namespace:
for (var i = 1; i < partCount; i++)
{
diagnostics.Add(ErrorCode.ERR_DuplicateNameInNS, declaration.Declarations[i].NameLocation, self.Name, self.ContainingSymbol);
diagnostics.Add(ErrorCode.ERR_DuplicateNameInNS, declaration.Declarations[i].NameLocation, this.Name, this.ContainingSymbol);
modifierErrors = true;
}
break;
Expand All @@ -397,7 +395,7 @@ private DeclarationModifiers MakeAndCheckTypeModifiers(
for (var i = 1; i < partCount; i++)
{
if (ContainingType!.Locations.Length == 1 || ContainingType.IsPartial())
diagnostics.Add(ErrorCode.ERR_DuplicateNameInClass, declaration.Declarations[i].NameLocation, self.ContainingSymbol, self.Name);
diagnostics.Add(ErrorCode.ERR_DuplicateNameInClass, declaration.Declarations[i].NameLocation, this.ContainingSymbol, this.Name);
modifierErrors = true;
}
break;
Expand All @@ -411,16 +409,32 @@ private DeclarationModifiers MakeAndCheckTypeModifiers(
var mods = singleDeclaration.Modifiers;
if ((mods & DeclarationModifiers.Partial) == 0)
{
diagnostics.Add(ErrorCode.ERR_MissingPartial, singleDeclaration.NameLocation, self.Name);
diagnostics.Add(ErrorCode.ERR_MissingPartial, singleDeclaration.NameLocation, this.Name);
modifierErrors = true;
}
}
}
}

if (this.Name == SyntaxFacts.GetText(SyntaxKind.RecordKeyword))
{
var location = declaration.Declarations[0].NameLocation;
Copy link
Member

@333fred 333fred Aug 26, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {}
``` #Resolved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

var nameText = location.SourceTree.GetRoot().FindToken(location.SourceSpan.Start).ToString();
ReportTypeNamedRecord(nameText, this.DeclaringCompilation, diagnostics, location);
}

return result;
}

static internal void ReportTypeNamedRecord(string name, CSharpCompilation compilation, DiagnosticBag diagnostics, Location location)
{
if (name == SyntaxFacts.GetText(SyntaxKind.RecordKeyword) &&
compilation.LanguageVersion >= MessageID.IDS_FeatureRecords.RequiredVersion())
{
diagnostics.Add(ErrorCode.WRN_RecordNamedDisallowed, location, name);
}
}

#endregion

#region Completion
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,8 @@ private ImmutableArray<TypeParameterSymbol> MakeTypeParameters(DiagnosticBag dia
}
}

ReportTypeNamedRecord(tp.Identifier.Text, this.DeclaringCompilation, diagnostics, location);

if (!ReferenceEquals(ContainingType, null))
{
var tpEnclosing = ContainingType.FindEnclosingTypeParameter(name);
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,8 @@ private ImmutableArray<TypeParameterSymbol> MakeTypeParameters(MethodDeclaration
}
}

SourceMemberContainerTypeSymbol.ReportTypeNamedRecord(identifier.Text, this.DeclaringCompilation, diagnostics, location);

var tpEnclosing = ContainingType.FindEnclosingTypeParameter(name);
if ((object)tpEnclosing != null)
{
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2504,6 +2504,16 @@
<target state="new">Type does not implement the collection pattern; member is is not a public instance or extension method.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed_Title">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ShouldNotReturn">
<source>A method marked [DoesNotReturn] should not return.</source>
<target state="translated">Metoda označená jako [DoesNotReturn] by neměla vracet hodnotu</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2504,6 +2504,16 @@
<target state="new">Type does not implement the collection pattern; member is is not a public instance or extension method.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed_Title">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ShouldNotReturn">
<source>A method marked [DoesNotReturn] should not return.</source>
<target state="translated">Eine mit [DoesNotReturn] gekennzeichnete Methode darf nicht zurückgegeben werden.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2504,6 +2504,16 @@
<target state="new">Type does not implement the collection pattern; member is is not a public instance or extension method.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed_Title">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ShouldNotReturn">
<source>A method marked [DoesNotReturn] should not return.</source>
<target state="translated">Un método marcado [DoesNotReturn] no debe devolver.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2504,6 +2504,16 @@
<target state="new">Type does not implement the collection pattern; member is is not a public instance or extension method.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed_Title">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ShouldNotReturn">
<source>A method marked [DoesNotReturn] should not return.</source>
<target state="translated">Une méthode marquée [DoesNotReturn] ne doit pas être retournée.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2504,6 +2504,16 @@
<target state="new">Type does not implement the collection pattern; member is is not a public instance or extension method.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed_Title">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ShouldNotReturn">
<source>A method marked [DoesNotReturn] should not return.</source>
<target state="translated">Un metodo contrassegnato con [DoesNotReturn] non deve essere restituito.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2504,6 +2504,16 @@
<target state="new">Type does not implement the collection pattern; member is is not a public instance or extension method.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed_Title">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ShouldNotReturn">
<source>A method marked [DoesNotReturn] should not return.</source>
<target state="translated">[DoesNotReturn] とマークされたメソッドを返すことはできません。</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2504,6 +2504,16 @@
<target state="new">Type does not implement the collection pattern; member is is not a public instance or extension method.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed_Title">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ShouldNotReturn">
<source>A method marked [DoesNotReturn] should not return.</source>
<target state="translated">[DoesNotReturn]으로 표시된 메서드는 반환하지 않아야 합니다.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2504,6 +2504,16 @@
<target state="new">Type does not implement the collection pattern; member is is not a public instance or extension method.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed_Title">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ShouldNotReturn">
<source>A method marked [DoesNotReturn] should not return.</source>
<target state="translated">Metoda oznaczona [DoesNotReturn] nie powinna zwracać wartości.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2502,6 +2502,16 @@
<target state="new">Type does not implement the collection pattern; member is is not a public instance or extension method.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed_Title">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ShouldNotReturn">
<source>A method marked [DoesNotReturn] should not return.</source>
<target state="translated">Um método marcado como [DoesNotReturn] não deve ser retornado.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ru.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2504,6 +2504,16 @@
<target state="new">Type does not implement the collection pattern; member is is not a public instance or extension method.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed_Title">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ShouldNotReturn">
<source>A method marked [DoesNotReturn] should not return.</source>
<target state="translated">Метод, помеченный [DoesNotReturn], не должен возвращать значение.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.tr.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2504,6 +2504,16 @@
<target state="new">Type does not implement the collection pattern; member is is not a public instance or extension method.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed_Title">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ShouldNotReturn">
<source>A method marked [DoesNotReturn] should not return.</source>
<target state="translated">[DoesNotReturn] olarak işaretlenen bir yöntem, döndürmemelidir.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hans.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2504,6 +2504,16 @@
<target state="new">Type does not implement the collection pattern; member is is not a public instance or extension method.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_RecordNamedDisallowed_Title">
<source>Types and aliases should not be named 'record'.</source>
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ShouldNotReturn">
<source>A method marked [DoesNotReturn] should not return.</source>
<target state="translated">不应返回标记为 [DoesNotReturn] 的方法。</target>
Expand Down
Loading