dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.92k stars 4.02k forks source link

IDE0055 triggered incorrectly with UNIX only line ending #55526

Open yangyud-cn opened 3 years ago

yangyud-cn commented 3 years ago

Version Used: "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin\Roslyn\csc.exe" Microsoft (R) Visual C# Compiler version 3.10.0-4.21269.26 (02984771) Copyright (C) Microsoft Corporation. All rights reserved.

Steps to Reproduce: This constantly popup during my project build. The fix is to change from UNIX "0x0a" to DOS "0x0d 0x0a" on that specific line only.

NLogLoggerGeneric.cs(8,50): error IDE0055: Fix formatting

image

sarahelsaig commented 2 years ago

If you are using git, a possible but inelegant workaround is adding

# Enforce Windows newlines for C# files to avoid false positives with IDE0055 warning.
*.cs text eol=crlf

to the .gitattributes file, and then calling git add --renormalize .. The trouble is, many Linux IDEs will try to convert your files to the native LF line endings when you save. So this workaround is more useful for code analysis in the CI, not so much for local development.

Also I'm not sure if this is relevant, but I was hoping that setting end_of_line = lf in the .editorconfig file would affect this. Unfortunately it doesn't.

JoeRobich commented 2 years ago

In roslyn we do something similar, in our .gitattributes file we set our code files to have their line endings automatically managed by git. In our .editorconfig we do not specify a end_of_line, since that will be dependent on platform and will be normalized by git when committing changes.

https://github.com/dotnet/roslyn/blob/326d841d38f70b2408471aa3429b9902a1b2609c/.gitattributes#L1-L15

JoeRobich commented 2 years ago

@yangyud-cn Sorry it has been so long without a response. Are you allowing git to automatically manage your end of line characters? Or, are you trying to manage end of line in your .editorconfig?

sarahelsaig commented 2 years ago

We were doing that too at first. It worked fine while everyone only worked on Windows. We only noticed this problem once we started to test on Linux too. If you let Git manage the line endings with text=auto, then it checks the files out with native line endings (LF on Linux and CRLF on Windows), and commits upstream as LF. So you get the error described in this issue when using Linux, but not on Windows.

JoeRobich commented 2 years ago

@DAud-IcI Roslyn has been using this configuration without issue for years with developers committing from many platforms. My understanding is that after renormalization all platforms would commit upstream with LF line endings.

0liver commented 2 years ago

Correct.

DerekSMorin commented 2 years ago

I will add that as a user, I wanted to turn on compile errors on code formatting warnings to make sure I was getting good code formatting and prevent others from committing bad indentation etc to my repo. But I was getting inconsistencies on my local compiles vs devops compiles due to git and line endings being different on my computer vs in devops. If I could rely on compiles being identical on my machine vs devops ( with the different line endings ) it wouldn't be a big deal because I'd compile locally, fix things and commit.

When I get stuff that compiles locally but fails in devops it drives me batty so I ended up just turning off the compile errors on code formatting warnings. If stuff like this doesn't just "work" I'm not sure how many hoops I'd want to jump through to get it going.

And I say this as someone who WAS very excited about turning on code formatting warnings as compile errors to prevent bad checkins to devops :)

JoeRobich commented 2 years ago

@DerekSMorin My recommendation is to add a .gitattributes file to your repo that will let git automatically handle line endings.

 ############################################################################### 
 # Set default behavior to automatically normalize line endings. 
 ############################################################################### 
 * text=auto encoding=UTF-8 

Remove end_of_line from your .editorconfig if you have it defined.

Then run git add --renormalize . to renormalize all your line endings and push the changes. You team will then need to pull changes to work with the normalized line endings.

0liver commented 2 years ago

@JoeRobich That's what we initially used, too. But please read @DAud-IcI's comment again - this solution will fail you on any Linux machine. And that's what we run our GitHub Actions on nowadays. Didn't use to be a problem on TeamCity on Windows Server before, but here we are.

That's why we currently work around this problem by adding this line to the .gitattributes in all of our repos:

# Enforce Windows newlines for C# files to avoid false positives with IDE0055 warning.
*.cs text eol=crlf

(as mentioned by @DAud-IcI in an earlier comment)

CyrusNajmabadi commented 2 years ago

It's unclear to me why anyone would want their source files to change based on the machine they're on. It feels like it would break even basic stuff like determinism and whatnot. Why not just use the exact same content on all platforms?

(it's like having a system that changed file encoding depending on what you were on. e.g. utf8 on some machines, and utf16 on others. it just seems bizarre and likely to cause problems.).

0liver commented 2 years ago

Imagine all the tooling under Windows or Unix that works with files - and uses CRLF or LF for new lines because that's the standard on that given system. Would you like to work with LF newlines under Windows? Or CRLF under Linux? I suppose no. The type of newline should not matter to the rest of the file contents.

That's also why *.cs text eol=crlf is an ugly workaround, made necessary due to flaws in the tooling.

CyrusNajmabadi commented 2 years ago

Would you like to work with LF newlines under Windows? Or CRLF under Linux?

I do both and have no issue with it.

The type of newline should not matter to the rest of the file contents.

Definitionally it matters, and it matters greatly. If I have a verbatim string, then that newline is encoded into the final program content.

Similarly, all debugging info is against offsets and positions which aren't the same if newlines change.

As above, imagine if you had to switch the encodings of all your files from utf8 to utf16 when moving from one OS to another. It would be terrible. Don't do it, you doing need to.

yangyud-cn commented 2 years ago

@JoeRobich the problem is that file itself is all Unix line ending as we always enforced that during git checkin as well as check out. Then in VS, I have to specifically change these two lines to DOS line ending to fix that warning. This makes the file a mixed line ending one. The workaround of auto line ending doesn't look a good one from my point of view.

DerekSMorin commented 2 years ago

This has generated a bit of discussion. I'm hoping this will be fixed, or there will be a way to ignore line endings or suppress these errors.

CyrusNajmabadi commented 2 years ago

afaict, this is working as expected. Teh newlines are incorrect in the file and the formatter is complaining. My recommendation would be to not have the tools change the content of code as that very much changes the meaning of it across many tools. It also breaks things like determinism/etc. all of which depend on the exact byte-contents of a file being the same everywhere.

0liver commented 2 years ago

Are Windows-style newlines really a must-have for the formatter to work correctly? Without knowing the internals, this sounds opinionated at best. It also doesn't fit into Microsoft's (.NET Core) ambitions to make *nix systems a first-class citizen.

CyrusNajmabadi commented 2 years ago

Are Windows-style newlines really a must-have for the formatter to work correctly?

The formatter should be respecting what is in the editorconfig for the project.

It also doesn't fit into Microsoft's (.NET Core) ambitions to make *nix systems a first-class citizen.

*nix is a first class citizen. The issue here is that tools that people are using seem to be converting the newlines, leading to the source files being genuinely different on different machines. If your formatting settings say that crlf should be used, but then your tools convert hte files to lf, then there will be a problem.

Note: as before, i strongly discourage having your tools convert teh line endings here. It literally changes the meaning of your code. Your builds will now be different and inconsistent across machines. AFAICT, all tools nowadays work fine with just lf line endings, or crlf line endings. So just standardize on one and then things will be consistent everywhere. Or, if you do want to have different endings on different machines, ensure that your formatting settings are correspondingly different as well.

--

For example, imagine if you had a tool that automatically converted spaces-to-tabs (or vice versa) when checking out on different people's machines. And you then had a formatting setting saying "i want this all to be spaces". Then you would definitely get issues. The solution is to either not unnecessarily convert, or to not have formatting rules set to oppose your conversion rules.

--

Finally, note that Roslyn itself is a project exactly like what is described here. We have people on Linux, OSX and Windows. And our CI machines also run on a combination of those OSs. We enforce formatting, but don't have an issue here using hte approach already outlined in this thread above.

sharwell commented 2 years ago

Are Windows-style newlines really a must-have for the formatter to work correctly?

The formatter should be respecting what is in the editorconfig for the project.

In general, .editorconfig should not specify line endings, and users should use the system line endings. In the rare case where a user needs a particular line ending, in would need to be configured in both .gitattributes and .editorconfig, otherwise some clients would ignore the setting.

sharwell commented 2 years ago

But please read https://github.com/dotnet/roslyn/issues/55526#issuecomment-1125491707 again - this solution will fail you on any Linux machine. And that's what we run our GitHub Actions on nowadays. Didn't use to be a problem on TeamCity on Windows Server before, but here we are.

Linux builds enforce LF line endings by default, and Windows builds enforce CRLF by default. The configuration described by @JoeRobich in https://github.com/dotnet/roslyn/issues/55526#issuecomment-1151428794 will adhere to this on both Linux and Windows.

DerekSMorin commented 2 years ago

I have windows line endings on my machine, and when it gets committed and pushed to devops I have linux line endings.

I think that is happening because of git settings.

So I'm confused on what the recommended approach is. What am I doing wrong?

I'm also not sure why I see only a few errors instead of lots of errors.

I put some vscode screenshots here:

https://github.com/dotnet/roslyn/issues/61042

Piedone commented 2 years ago

Putting this frustrating matter aside, I welcome everyone to reflect on the amusing fact that now, in 2022, we have this bug report for the C# compiler's static code analysis feature because 60 years ago some people needed to instruct electromechanical teleprinters.

sharwell commented 2 years ago

I have windows line endings on my machine, and when it gets committed and pushed to devops I have linux line endings.

I think that is happening because of git settings.

So I'm confused on what the recommended approach is. What am I doing wrong?

I'm also not sure why I see only a few errors instead of lots of errors.

I put some vscode screenshots here:

61042

There are three reasons why this could be occurring:

  1. Not all files have Unix-style endings in the CI build (this can happen if one or more files in source control is not normalized, or if .gitattributes is missing text=auto for the line that applies to *.cs files)
  2. The CI build is running on a system where Environment.NewLine is \r\n
  3. One or more .editorconfig files applied to the build specifies an end_of_line value
Varorbc commented 8 months ago

Remove end_of_line from your .editorconfig if you have it defined.

@JoeRobich I had removed the end_of_line definition, but when I opened the .editorconfig editor again, it was added back.

Should the.editorconfig editor not automatically add an end_of_line definition?

JoeRobich commented 8 months ago

Should the .editorconfig editor not automatically add an end_of_line definition?

@sharwell If the editor is always saving a value here, it seems like this might be an easy setting for the editor to get wrong.

sharwell commented 8 months ago

The editor should not add end_of_line to the text of the saved file unless it either 1) was already present in the file when it was opened or 2) the user has modified the value for it in the editor. I'm guessing it does not currently adhere to this expectation, but that would be a separate bug.

JoeRobich commented 8 months ago

I think the question was more on Roslyn's .editorconfig UI editor experience. The user had removed the end_of_line setting but the editor seems to have added one back the next time they opened it.

sharwell commented 8 months ago

Right, that's a bug in the .editorconfig designer.

Neme12 commented 6 months ago

@yangyud-cn @DerekSMorin Can you confirm whether or not this issue persists if you a) remove end_of_line=crlf from your editorconfig if this is there, or b) if you add end_of_line=lf?