False positive RS1035 error for Environment.NewLine

[bitbonk]

Analyzer

Diagnostic ID: RS1035 The symbol '{0}' is banned for use by analyzers: Analyzers should not read their settings directly from environment variables

Analyzer source

Unknown, I am using SDK 6.0.405 and the following Nuget packages

Microsoft.CodeAnalysis.CSharp 4.3.0 Microsoft.CodeAnalysis.Analyzers 3.3.4 Microsoft.CodeAnalysis.Workspaces.Common 4.3.0

Describe the bug

When I use Environment.NewLine inside a source generator, I get the error

Error RS1035 : The symbol 'Environment' is banned for use by analyzers: Analyzers should not read their settings directly from environment variables.

I understand that it maybe be dangerous to read settings from environment variables but the class Environment has members that AFAIK have nothing to do with environment variables and therefore should be save to use.

In the case of Environment.NewLine, what is the right way add a newline dynamically in generated code?

Steps To Reproduce

1. Create a source generator using the SDK and nuget pakages mentioned above
2. Use Environment.NewLine anywhere in the generator,
3. See error

Expected behavior

No error occurs when I use Environment.NewLine

Actual behavior

The error

Error RS1035 : The symbol 'Environment' is banned for use by analyzers: Analyzers should not read their settings directly from environment variables

occurs.

[mavasani]

@RikkiGibson

[bitbonk]

Speaking of Enironment.NewLine: It might not be the right thing to use in the source text generated by source generator anyway: https://github.com/dotnet/roslyn/issues/51437

[RikkiGibson]

Thanks for pinging on this issue. I think the expectation is that we get the preferred newline style from the project we are generating for instead of picking the OS-level default for the source generator's own build environment. Do I have that right @jaredpar?

FWIW, it seems extremely easy to include newlines in the generated code which don't match the user project's expectations, by using a @" or """ literal string. It does seem to me that if we can't come up a coding pattern which prevents such newlines from getting introduced, or replaces them systematically, we should probably remove the error on Environment.NewLine.

Assuming this error is expected, the error message could be improved here, since it mainly had GetEnvironmentVariable, etc. in mind when it was written.

[eatdrinksleepcode]

I think the expectation is that we get the preferred newline style from the project we are generating...

@RikkiGibson How would a generator do that?

It is extremely frustrating that Microsoft.CodeAnalysis.Analyzers (which is included automatically by Microsoft.CodeAnalysis.CSharp) raises these build errors by default, yet there is almost no documentation or guidance about how to fix them.

[jmarolf]

In an analyzer/source generator I would expect people to do:

var syntaxTree = ... // syntaxTree you want to know the newline settings for
var options = context.AnalyzerConfigOptions.GetOptions(syntaxTree);
if (options.TryGetValue("end_of_line", out var newline))
{
    // do something with newline
}

If you are in a workspace context I would expect:

var workspace = ... // get a reference to a workspace
var newline = workspace.GetOption(new OptionKey(FormattingOptions.NewLine, LanguageNames.CSharp));

But that is if you would actually do anything different based on the value of this setting.

If you are generating code and want it to just be "whatever the users settings say" then use SyntaxFactory.ElasticCarriageReturnLineFeed

[sharwell]

For a source generator, it would also be acceptable to just use \n (the file never exists on disk or in source control).

[jmarolf]

The core problem in a source generator is that if you use Enironment.NewLine then the build will not be deterministic. The compiler output changes based on environmental factors that are unrelated to the compilation. If I have identical inputs to the compiler but am on a machine with different values for Enironment.NewLine I will get different binary output.

[RikkiGibson]

Where should we record the above advice to help people who encounter this warning?

[Youssef1313]

The core problem in a source generator is that if you use Enironment.NewLine then the build will not be deterministic. The compiler output changes based on environmental factors that are unrelated to the compilation. If I have identical inputs to the compiler but am on a machine with different values for Enironment.NewLine I will get different binary output.

@jmarolf The recommendation for new line handling has always been to not specify "end_of_line" in editorconfig, and let git normalize whitespaces, which means that if a codebase contains multiline raw/verbatim string literals, they will end up being either \n or \r\n based on environmental factors (e.g, Windows vs Unix).

Why/how is the case for source generators any different?

[RikkiGibson]

I'm wondering if we should unban NewLine here.

Separately, I'm also wondering if we should unban CurrentCulture, at least until we have a clear pattern we want people to use instead.

[AArnott]

This rule is super problematic for source generators that use T4 for generation because the runtime code generated by the T4 system includes references to both Environment.NewLine and CurrentCulture. I get why these two are bad APIs for determinism, but I feel like this being the case, and T4 being one of the solutions for source generators, the roslyn and T4 teams should get together to solve this (presumably by changing how T4 does its code gen, at least under a setting).

[StephenDrewLDS]

I wanted to include a newline in a diagnostic descriptor message, but apparently this is also forbidden.

[FreeApophis]

So, we also ran into this when we updated the Microsoft.CodeAnalysis.CSharp - what a nice, unexpected, unnecessary breaking change.

[jrmoreno1]

@jmarolf:

The core problem in a source generator is that if you use Enironment.NewLine then the build will not be deterministic. The compiler output changes based on environmental factors that are unrelated to the compilation. If I have identical inputs to the compiler but am on a machine with different values for Enironment.NewLine I will get different binary output.

Why would you get non-deterministic build? Enironment.NewLine is a function call into System.Runtime, it should be a function call into System.Runtime on all frameworks and for any compiler. The Jitter might optimize it, but I wouldn't expect the compiler to do so.

EDIT: Ah, never mind, I believe you are saying that the TARGET of the source generator would result in a non-deterministic build.

[agocke]

Ping on this -- I think the correct fix here is to remove the warning for Environment.NewLine in the analyzer. Even if Environment.NewLine is not the best possible choice, it's not a bad choice, and the analyzer's justification for warning here is simply incorrect.