ErrorProne.NET icon indicating copy to clipboard operation
ErrorProne.NET copied to clipboard

EPS12 produces false positive on methods of non-readonly structs

Open shuebner opened this issue 4 years ago • 5 comments

EPS12 falsely reports that a method can be readonly on a non-readonly struct. This is nonsense, since there is no such thing (yet) as a readonly method.

A picture says more than a thousand words ;-): EPS12_false_positive

It even suggests and applies a non-compiling code-fix. Maybe this is something to be looked into in as well.

And the same happens on a property that is already read-only: image

In addition I saw once (not reproducible, but maybe it is related) this exception thrown from the analyzer:

CSC : warning AD0001: Analyzer 'ErrorProne.NET.StructAnalyzers.MakeStructMemberReadOnlyAnalyzer' threw an exception of type 'System.InvalidCastException' with message 'Unable to cast object of type 'Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE.PEAssemblySymbol' to type 'Microsoft.CodeAnalysis.CSharp.Symbols.SourceAssemblySymbol'.'.

shuebner avatar Oct 22 '21 03:10 shuebner

This is nonsense, since there is no such thing (yet) as a readonly method.

It's not that non-sensical, because C# has this feature for almost 2 years;). Try setting language version to 8.0 and the "nonsense code" would suddenly compile:)

In addition I saw once (not reproducible, but maybe it is related) this exception thrown from the analyzer:

Why to you think it maybe related? If you share a full stacktrace, I can look into this issue, but otherwise it's quite hard to figure out what went wrong.

SergeyTeplyakov avatar Oct 23 '21 20:10 SergeyTeplyakov

This is nonsense, since there is no such thing (yet) as a readonly method.

It's not that non-sensical, because C# has this feature for almost 2 years;). Try setting language version to 8.0 and the "nonsense code" would suddenly compile:)

Yes, you are of course right.

Friday was apparently my "documentation overlook day", because this is the second issue in which I overlooked existing documentation despite having explicitly searched for it. Also, I overlooked the fact that the project in which the warning occurred is the only one in my solution that does not declare language version 9.0 🙄 .

However, the diagnostic and code fix still should not have been displayed for C# 7.3.

In addition I saw once (not reproducible, but maybe it is related) this exception thrown from the analyzer:

Why to you think it maybe related? If you share a full stacktrace, I can look into this issue, but otherwise it's quite hard to figure out what went wrong.

I do not assume anything. I just observed the exception while dealing with the non-compiling code fix and wanted to give as much context as possible. I also do not have a stack trace as I could not reproduce the exception.

shuebner avatar Oct 25 '21 04:10 shuebner

However, the diagnostic and code fix still should not have been displayed for C# 7.3.

I filed a bug (#247) to not emit a diagnostic if the fix is not available in the language used by the project.

I do not assume anything. I just observed the exception while dealing with the non-compiling code fix and wanted to give as much context as possible. I also do not have a stack trace as I could not reproduce the exception.

I'll try to play around with the case you describe and maybe I'll be able to repo the issue, but if you'll hit it again, I think the 'Error List' in Visual Studio should have a full stacktrace of the issue. Have a stacktrace will make the fix trival.

Thanks a lot for making this project better!

SergeyTeplyakov avatar Oct 25 '21 22:10 SergeyTeplyakov

I think the 'Error List' in Visual Studio should have a full stacktrace of the issue.

That is exactly my point, it does not. And I have not found any setting that could give it to me. I have the same problem when developing my own analyzers and source generators.

I observed the same exception again when I freshly added the analyzer to a project that triggered this EPS12 diagnostic. Maybe that is how you could reproduce.

shuebner avatar Oct 26 '21 03:10 shuebner

I observed the same exception again when I freshly added the analyzer to a project that triggered this EPS12 diagnostic. Maybe that is how you could reproduce.

It is strange that you can't see the stacktrace in the error list, because I have seen them in a few cases, but you have a good point and I'll try to repro the issue the same way you got it the first time. The concern is that the state of the source code forces this conversion error, but I'll try checking various codebases and I hope to hit this issue at least on one of them.

SergeyTeplyakov avatar Oct 26 '21 08:10 SergeyTeplyakov