Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -7824,4 +7824,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_CollectionExpressionImmutableArray" xml:space="preserve">
<value>This version of '{0}' cannot be used with collection expressions.</value>
</data>
<data name="ERR_InvalidExperimentalDiagID" xml:space="preserve">
<value>The diagnosticId argument to the 'Experimental' attribute must be a valid identifier</value>
</data>
</root>
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2279,6 +2279,8 @@ internal enum ErrorCode
WRN_CollectionExpressionRefStructSpreadMayAllocate = 9209,
ERR_CollectionExpressionImmutableArray = 9210,

ERR_InvalidExperimentalDiagID = 9211,

#endregion

// Note: you will need to do the following after adding warnings:
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2409,6 +2409,7 @@ internal static bool IsBuildOnlyDiagnostic(ErrorCode code)
case ErrorCode.WRN_CollectionExpressionRefStructMayAllocate:
case ErrorCode.WRN_CollectionExpressionRefStructSpreadMayAllocate:
case ErrorCode.ERR_CollectionExpressionImmutableArray:
case ErrorCode.ERR_InvalidExperimentalDiagID:
return false;
default:
// NOTE: All error codes must be explicitly handled in this switch statement
Expand Down
8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Symbol_Attributes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,14 @@ protected void DecodeWellKnownAttribute(ref DecodeWellKnownAttributeArguments<At
return;
}

if (arguments.Attribute.IsTargetAttribute(this, AttributeDescription.ExperimentalAttribute)
&& !SyntaxFacts.IsValidIdentifier((string?)arguments.Attribute.CommonConstructorArguments[0].ValueInternal))
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 20, 2023

Choose a reason for hiding this comment

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

&& !SyntaxFacts.IsValidIdentifier((string?)arguments.Attribute.CommonConstructorArguments[0].ValueInternal))

Similar refactoring suggestion as for VB #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

On the second thought, never mind. It looks like we do want to call DecodeWellKnownAttributeImpl below.

{
var attrArgumentLocation = arguments.Attribute.GetAttributeArgumentSyntaxLocation(parameterIndex: 0, arguments.AttributeSyntaxOpt);
arguments.Diagnostics.DiagnosticBag.Add(ErrorCode.ERR_InvalidExperimentalDiagID, attrArgumentLocation);
return;
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 20, 2023

Choose a reason for hiding this comment

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

return

Shouldn't we still call DecodeWellKnownAttributeImpl below? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. The decoding of experimental attribute is done in EarlyDecodeWellKnownAttributes, before we get to DecodeWellKnownAttribute

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. The decoding of experimental attribute is done in EarlyDecodeWellKnownAttributes, before we get to DecodeWellKnownAttribute

First, the code should be robust to future changes and it feels reasonable to me that the fact you are referring to, if it is a fact, won't hold in the future.

Second, I actually see decoding done in DecodeWellKnownAttributeImpl for a SourceModuleSymbol today, for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you're right. The return broke things.

}

DecodeWellKnownAttributeImpl(ref arguments);
}
#nullable disable
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ru.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.tr.xlf

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

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

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

119 changes: 119 additions & 0 deletions src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14875,6 +14875,125 @@ public static void M() { }
var comp = CreateCompilation(src, options: TestOptions.DebugExe.WithGeneralDiagnosticOption(ReportDiagnostic.Suppress));
comp.VerifyDiagnostics();
}

[Fact]
public void ExperimentalWithWhitespaceDiagnosticID_WarnForInvalidDiagID()
{
var dir = Temp.CreateDirectory();
var src = dir.CreateFile("test.cs").WriteAllText("""
C.M();

[System.Diagnostics.CodeAnalysis.Experimental(" ")]
public class C
{
public static void M() { }
}

namespace System.Diagnostics.CodeAnalysis
{
public sealed class ExperimentalAttribute : Attribute
{
public ExperimentalAttribute(string diagnosticId) { }
}
}
""");
var analyzerConfig = dir.CreateFile(".editorconfig").WriteAllText("""
[*.cs]
dotnet_diagnostic.CS9211.severity = warning
""");
Assert.Equal((ErrorCode)9211, ErrorCode.ERR_InvalidExperimentalDiagID);
var cmd = CreateCSharpCompiler(null, dir.Path, new[] {
"/nologo",
"/t:exe",
"/preferreduilang:en",
"/analyzerconfig:" + analyzerConfig.Path,
src.Path });

var outWriter = new StringWriter(CultureInfo.InvariantCulture);
var exitCode = cmd.Run(outWriter);
Assert.Equal(1, exitCode);
Assert.StartsWith("test.cs(3,47): error CS9211: The diagnosticId argument to the 'Experimental' attribute must be a valid identifier",
outWriter.ToString());
}

[Fact]
public void ExperimentalWithValidDiagnosticID_WarnForDiagID()
{
var dir = Temp.CreateDirectory();
var src = dir.CreateFile("test.cs").WriteAllText("""
C.M();

[System.Diagnostics.CodeAnalysis.Experimental("DiagID")]
public class C
{
public static void M() { }
}

namespace System.Diagnostics.CodeAnalysis
{
public sealed class ExperimentalAttribute : Attribute
{
public ExperimentalAttribute(string diagnosticId) { }
}
}
""");
var analyzerConfig = dir.CreateFile(".editorconfig").WriteAllText("""
[*.cs]
dotnet_diagnostic.DiagID.severity = warning
""");
var cmd = CreateCSharpCompiler(null, dir.Path, new[] {
"/nologo",
"/t:exe",
"/preferreduilang:en",
"/analyzerconfig:" + analyzerConfig.Path,
src.Path });

var outWriter = new StringWriter(CultureInfo.InvariantCulture);
var exitCode = cmd.Run(outWriter);
Assert.Equal(0, exitCode);
Assert.StartsWith("test.cs(1,1): warning DiagID: 'C' is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.",
outWriter.ToString());
}

[Fact]
public void ExperimentalWithValidDiagnosticID_WarnForExperimental()
{
var dir = Temp.CreateDirectory();
var src = dir.CreateFile("test.cs").WriteAllText("""
C.M();

[System.Diagnostics.CodeAnalysis.Experimental("DiagID")]
public class C
{
public static void M() { }
}

namespace System.Diagnostics.CodeAnalysis
{
public sealed class ExperimentalAttribute : Attribute
{
public ExperimentalAttribute(string diagnosticId) { }
}
}
""");
var analyzerConfig = dir.CreateFile(".editorconfig").WriteAllText("""
[*.cs]
dotnet_diagnostic.CS9204.severity = warning
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 19, 2023

Choose a reason for hiding this comment

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

CS9204

Consider asserting that the number corresponds to WRN_Experimental #Closed

""");
Assert.Equal((ErrorCode)9204, ErrorCode.WRN_Experimental);
var cmd = CreateCSharpCompiler(null, dir.Path, new[] {
"/nologo",
"/t:exe",
"/preferreduilang:en",
"/analyzerconfig:" + analyzerConfig.Path,
src.Path });

var outWriter = new StringWriter(CultureInfo.InvariantCulture);
var exitCode = cmd.Run(outWriter);
Assert.Equal(1, exitCode);
Assert.StartsWith("test.cs(1,1): error DiagID: 'C' is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.",
outWriter.ToString());
}
}

[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
Expand Down
Loading