Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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) #868

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ProductiveRage
Copy link

@ProductiveRage ProductiveRage commented Apr 27, 2021

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.

…e with immutable language-ext types (the results of the "Add" methods are discarded, which is bad)
@louthy
Copy link
Owner

louthy commented May 17, 2021

@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.

@ProductiveRage
Copy link
Author

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).

@louthy
Copy link
Owner

louthy commented May 17, 2021

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.

@ProductiveRage
Copy link
Author

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

…analyser only and doesn't pull the analyser's dependencies into the NuGet-ingesting project
@Lonli-Lokli
Copy link
Contributor

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

@ProductiveRage
Copy link
Author

@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!

@Lonli-Lokli
Copy link
Contributor

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

@ProductiveRage
Copy link
Author

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
Copy link
Author

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).

@StefanBertels
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants