Report a level 7 warning for lower-cased type names by jcouv · Pull Request #56905 · dotnet/roslyn

jcouv · 2021-10-01T06:57:20Z

LDM discussion 10/13/2021.

See test OnlyOneParse_WithReservedTypeName for why the check was moved out of symbol creation (we want to avoid pulling on the syntax tree more than once).

Youssef1313 · 2021-10-01T19:07:39Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs

+                    {
+                        if (!GetName(decl.SyntaxReference.GetSyntax()).Text.StartsWith("@"))
+                        {
+                            diagnostics.Add(ErrorCode.WRN_LowerCaseTypeName, decl.Location, declaration.Name);


Worth adding a code fix provider (IDE-side) for this warning? I'm not sure.

jcouv · 2021-11-30T06:01:42Z

@333fred On the day of LDM I'd updated the notes in OP to include types, aliases and namespaces. But the meeting notes only mention types, so I'm having doubts on whether I misunderstood the decision. Do you remember?

333fred · 2021-11-30T19:16:35Z

@333fred On the day of LDM I'd updated the notes in OP to include types, aliases and namespaces. But the meeting notes only mention types, so I'm having doubts on whether I misunderstood the decision. Do you remember?

I honestly don't remember whether we'd said anything about namespaces or aliases. I do think we should, but I don't remember if we did.

333fred · 2021-11-30T19:19:01Z

Can this implementation share any code/codepaths with https://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Symbols/Source/SourceMemberContainerSymbol.cs,455? IE, for records it'll give that specific warning, otherwise it'll give the general warning? Would give us less places to worry about duplicating checks.

jcouv · 2021-12-01T07:15:11Z

Can this implementation share any code/codepaths with ...

Indeed. We're already checking all these cases (type/type parameter/alias) for special name "record". Thanks

333fred · 2021-12-01T22:55:46Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

-        internal static void ReportTypeNamedRecord(string? name, CSharpCompilation compilation, DiagnosticBag? diagnostics, Location location)
+        internal static bool IsReservedTypeName(string? name)
+        {
+            return name is { Length: > 0 } && name.All(c => char.IsLower(c) && c < 128);


Why not just do c is >= 'a' and <= 'z'? #Resolved

I don't see a benefit (prefer readability over premature optimization). If anything, I wanted to use char.IsAscii instead of doing a lower-level check, but surprisingly it was introduced in .NET 6.

I specifically asked because I find the current code less clear than a simple character comparison. What is 128? I don't have the ascii table memorized. Far easier to avoid the confusion and simply say "is it between 'a' and 'z'?"

I don't think it's simpler. I can extract an isAscii local function (since char.IsAscii isn't available) if you'd like. Let me know.

I do not think the current code is at all clear, mainly because of the use of an unnamed constant 128. I think this code will be much simpler we do a standard char comparison.

It seems like c is >= 'a' and <= 'z' is checking something different than c < 128. It might be worth digging a bit to see if constant 128 is anywhere in our character encoding helpers, if you haven't already. Otherwise I think a local variable or function isAscii with a brief comment like "128 is the top of the ascii range" would be fine.

edit: It looks like I'm behind on the latest changes in this conversation. The current implementation which entirely elides the < 128 check seems fine.

Youssef1313 · 2021-12-06T17:00:48Z

docs/compilers/CSharp/Warnversion Warning Waves.md

 | CS8898 | 5 | [Static class used as the return type of a method in an interface type](https://github.com/dotnet/roslyn/issues/38256) |
+| CS8981 | 7 | [Type names only containing lower-cased ascii characters may become reserved for the language](https://github.com/dotnet/roslyn/issues/56653) |


Is warnlevel 6 not documented? Is there a tracking issue for that? #Resolved

Filed #58132

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

Youssef1313 · 2021-12-06T17:14:44Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests.cs

            var compilation = CreateCompilationWithMscorlib40AndSystemCore(@"
 using System;
-static class m
+static class @m


Do you use an automation tool when a change affects many tests like this PR, or are these all manual changes? and if there is a tool, is it publicly available? (I hope this kind of unrelated questions don't bother you :) )

Manual changes.
If the IDE decides to add automation for this, it would still not help inside our tests unfortunately.

Youssef1313 · 2021-12-06T17:18:29Z

src/Compilers/CSharp/Test/Semantic/Semantics/MethodBodyModelTests.cs

            var text = @"
 using System;
-interface i1
+interface @i1


Why this is required? The character 1 isn't in the [a-z] range. #Fixed

Good catch. Those indeed can be left unescaped.

Note that the #Resolved and similar edits are inserted into our comments by an internal tool. If the goal is to match the conventions of that tool, then:

we commonly use the tag #Closed to indicate that the original author of the comment has confirmed the issue to be addressed

the tag #Resolved indicates the PR author has made changes to address someone's comment, but the original author of the comment must confirm that the issue was addressed before changing the state to #Closed.

Youssef1313 · 2021-12-06T17:18:45Z

src/Compilers/CSharp/Test/Semantic/Semantics/MethodBodyModelTests.cs

            var text = @"
 using System;
-public interface i1
+public interface @i1


Same here #Fixed

cston · 2021-12-06T23:24:43Z

src/Compilers/CSharp/Test/Symbol/Symbols/SymbolErrorTests.cs

+
+class A { }
+class Z { }
+


Consider testing class abcdefghijklmnopqrstuvwxyz { }

cston · 2021-12-06T23:27:02Z

src/Compilers/CSharp/Test/Symbol/Symbols/SymbolErrorTests.cs

+class A { }
+class Z { }
+
+// first lower-case letter outside ascii range


Perhaps split these cases that include parse errors into a separate test.

333fred

LGTM (commit 13)

jaredpar · 2021-12-07T16:04:38Z

Did we test this change against runtime to see how many violations it found?

jcouv · 2021-12-07T18:52:21Z

@jaredpar Haven't tried that. Let me get back to you later today.

jcouv · 2021-12-07T22:54:05Z

@jaredpar Here's the impact on runtime repo: dotnet/runtime#62507

ghost added the Area-Compilers label Oct 1, 2021

jcouv force-pushed the lower-case branch 2 times, most recently from d25d9c7 to 4e9ec71 Compare October 1, 2021 08:25

Youssef1313 reviewed Oct 1, 2021

View reviewed changes

jcouv and others added 3 commits November 29, 2021 17:56

Report a level 7 warning for lower-cased type names

99d660b

Adjust baselines

b287522

bootstrap

404fb7b

jcouv force-pushed the lower-case branch from e593420 to 404fb7b Compare November 30, 2021 01:59

check for ascii too

d8a63cf

Julien Couvreur added 3 commits November 30, 2021 14:59

Also check alises and type parameters

8429a34

Merge remote-tracking branch 'dotnet/main' into lower-case

47343b2

tweak

e8a1a42

jcouv force-pushed the lower-case branch from d5dffe1 to e8a1a42 Compare December 1, 2021 01:07

Only parse once if possible

0f9e867

jcouv force-pushed the lower-case branch from 6b1fb5a to 0f9e867 Compare December 1, 2021 06:52

Julien Couvreur added 2 commits November 30, 2021 22:59

Move check outside of symbol constructor

64c0eb2

Document warning wave

3f1fa57

runfoapp bot mentioned this pull request Dec 1, 2021

Transient failure in test AsyncIteratorWithCustomCode #34207

Closed

jcouv marked this pull request as ready for review December 1, 2021 15:47

jcouv requested a review from a team as a code owner December 1, 2021 15:47

333fred reviewed Dec 2, 2021

View reviewed changes

jcouv requested a review from a team December 6, 2021 15:58

Youssef1313 reviewed Dec 6, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs Show resolved Hide resolved

Youssef1313 reviewed Dec 6, 2021

View reviewed changes

Julien Couvreur added 3 commits December 6, 2021 09:23

Remove escape when a digit is in name

59caa2b

use char arithmetic

a5d60fa

escape

f8068af

runfoapp bot mentioned this pull request Dec 6, 2021

Failure in CSharpCodeActions.GFUFuzzyMatchAfterRenameTrackingAndAfterGenerateType #57423

Closed

cston reviewed Dec 6, 2021

View reviewed changes

cston approved these changes Dec 6, 2021

View reviewed changes

333fred approved these changes Dec 6, 2021

View reviewed changes

tweak

efe1a70

jcouv enabled auto-merge (squash) December 6, 2021 23:59

jcouv merged commit 0d68a4b into dotnet:main Dec 7, 2021

ghost added this to the Next milestone Dec 7, 2021

jcouv modified the milestones: Next, 17.1 Dec 7, 2021

jcouv deleted the lower-case branch December 7, 2021 05:18

runfoapp bot mentioned this pull request Dec 7, 2021

Test failure in Microsoft.CodeAnalysis.CSharp.Emit.UnitTests.dll #57560

Closed

This was referenced Dec 7, 2021

Enable max warnlevel from C# compiler dotnet/runtime#62506

Merged

Escape lower-cased type names dotnet/runtime#62507

Merged

CyrusNajmabadi mentioned this pull request Apr 12, 2022

Compiler or analyzer should warn for lower-cased type names #56653

Closed

		\| CS8898 \| 5 \| [Static class used as the return type of a method in an interface type](https://github.com/dotnet/roslyn/issues/38256) \|
		\| CS8981 \| 7 \| [Type names only containing lower-cased ascii characters may become reserved for the language](https://github.com/dotnet/roslyn/issues/56653) \|

Conversation

jcouv commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Youssef1313 Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Nov 30, 2021

Uh oh!

333fred commented Nov 30, 2021

Uh oh!

333fred commented Nov 30, 2021

Uh oh!

jcouv commented Dec 1, 2021

Uh oh!

333fred Dec 1, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Dec 6, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Youssef1313 Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Dec 7, 2021

Uh oh!

jcouv commented Dec 7, 2021

Uh oh!

jcouv commented Dec 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jcouv commented Oct 1, 2021 •

edited

Loading

Youssef1313 Oct 1, 2021 •

edited

Loading

333fred Dec 1, 2021 •

edited by jcouv

Loading

RikkiGibson Dec 6, 2021 •

edited

Loading

Youssef1313 Dec 6, 2021 •

edited by jcouv

Loading

Youssef1313 Dec 6, 2021 •

edited

Loading

Youssef1313 Dec 6, 2021 •

edited

Loading

Youssef1313 Dec 6, 2021 •

edited

Loading