language-ext icon indicating copy to clipboard operation
language-ext copied to clipboard

Create an analyser package that looks for collection initialiser usage with immutable language-ext types (the results of the "Add" methods are discarded, which is bad)

Open ProductiveRage opened this issue 3 years ago • 10 comments

This isn't quite a complete change because it might make sense to include the analysers as a dependency of the main package, so that if you install language-ext then you get the analysers as well (like I believe xUnit does).

Also, I wasn't sure if you would want to tweak the package name or the namespaces - I've tried to be consistent with what is used in the rest of the solution but I may have overlooked something.

The idea behind the single analyser that is included is that it detects when a language-ext type is "populated" by a collection initialiser ("populated" in quotes because it won't actually work) and records an error to inform the code writer:

Example analyser error

It only does this work for language-ext types, it doesn't prevent the collection initialiser from being used with the standard mutable BCL List type, for example.

Relates to this C# proposal that you raised: Opt-out attribute for collection initialization.

ProductiveRage avatar Apr 27 '21 22:04 ProductiveRage

@ProductiveRage Thanks for this Dan, the project naming is inconsistent with the other projects, so LanguageExt.Analysers would be better than LanguageExtAnalysers. It looks like it has dependencies we wouldn't want in the main project, so best to keep it separate I think.

louthy avatar May 17 '21 15:05 louthy

I'll look to change those name spaces for consistency.

In relation to the dependencies - analysers run within the context of Visual Studio and not the solution that you have open. So you can add as many analysers as you want, with whatever crazy tree of dependencies that they may have and those dependencies will not work their way into your solution code. Does that make sense? I think it's a non-issue but I might be getting my wires crossed. (It's worth having a look at the way that analysers are installed - via some .ps files that add them into the project in a special manner and not as a standard project / assembly reference whose dependencies would be pulled in).

ProductiveRage avatar May 17 '21 16:05 ProductiveRage

With the code-gen stuff you can declare dependencies that don't appear in the final build, which may be the same thing here. I've never built a Roslyn analyzer, so I'd need to get my head around the implications. The last thing I need is ImmutableCollections embedded in the Core lib. I'll give you a shout on Teams later in the week and we can go over it.

louthy avatar May 17 '21 16:05 louthy

Sounds good. And I'm totally behind ImmutableCollections not getting into the library which already has its own immutable collections!

ProductiveRage avatar May 17 '21 16:05 ProductiveRage

So you can add as many analysers as you want, with whatever crazy tree of dependencies that they may have and those dependencies will not work their way into your solution code.

Actually when user adds analyzer to solution it might gets added to application output, based on settings used with analyzers package

Lonli-Lokli avatar May 17 '21 22:05 Lonli-Lokli

@Lonli-Lokli - I think that's correct; that it's possible to have analyser assemblies appear in the build output of the application that has the analyser loaded into one (or more) of its projects however I don't believe that I've seen it before myself and it should certainly be something that can be prevented with the right options.

I've pushed another couple of changes to illustrate it, after testing locally - we'll see if it works properly for everyone!

ProductiveRage avatar May 17 '21 22:05 ProductiveRage

it should certainly be something that can be prevented with the right options.

Thats right, I mean that it's better to include correct way of using analyzer in README.

By correct I mean this steps

  1. Create Hello World app with pure Lst usage, run dotnet publish and see output
  2. add analyzer, run dotnet publish - output should be the same as in step 1

Lonli-Lokli avatar May 17 '21 22:05 Lonli-Lokli

Agreed. Hopefully, if the analyser is included in the main library (like the xunit analysers are) then no Readme will even necessary! We'll just test it out thoroughly and then it should be a seamless experience for anyone using the library!

ProductiveRage avatar May 17 '21 22:05 ProductiveRage

I realised that I didn't add a note here specifically to say that I tested as @Lonli-Lokli suggested and the build output of a project that includes the analyser is the same as one that doesn't - so the analyser dependencies do not end up in the build output of the project that includes the analyser, which is the result that we wanted to see (ie. no README or other setup details would be necessary to include this analyser functionality in this library).

ProductiveRage avatar Nov 16 '21 20:11 ProductiveRage

https://github.com/dotnet/roslyn-analyzers/issues/5685 https://github.com/dotnet/runtime/issues/34098

StefanBertels avatar May 20 '22 13:05 StefanBertels