Tracking: Issues & suggestions solvable with Analyzers SDK

T-Gro commented 1 year ago

This issue exists to collect existing painpoints which would be solved if an F# Analyzer solution existed.

Warn on unoptimal APIs:

All below could likely be a single analyzer with a project-defined .txt file listing the APIs to warn on Ban Assert.AreEqual(obj,obj) https://github.com/dotnet/fsharp/issues/14598 Warn non-generic collections https://github.com/dotnet/fsharp/issues/16259

Warn on combination of API + type arguments

Should be also doable with a single analyzer driven by a text file Ban ignore<function> https://github.com/dotnet/fsharp/issues/15880 Warning for equality on structs because of boxing https://github.com/dotnet/fsharp/issues/526

Warn specific language constructs

Likely an easy generalization is not possible, those will need their own logic for traversing the (un)typed tree. The constructs to target are well specifiable, analyzer could register for kinds of nodes to prune irrelevant code Ban early return from CEs https://github.com/dotnet/fsharp/issues/15759 Ban unnamed DU fields https://github.com/dotnet/fsharp/issues/15665

T-Gro commented 1 year ago

Meaning of the November assignment:

To spent time researching the needs and use cases, not really building it yet.

BoundedChenn31 commented 1 year ago

Is it only about F# specific issues? Otherwise I would throw in applicable Roslyn analyzers that are enabled by default. For example,

Also maybe optional analyzer to remind about backgroundTask instead of task. As far as I know equivalent analyzer for ConfigureAwait(false) is quite popular in C# among library authors. And some analyzers for ASP.NET (sorry, couldn't find better link). Can be useful even when using giraffe.

Lanayx commented 1 year ago

Some suggestions from my work experience

Old patterns that can be covered by new features is the most desired suggestion
Avoid name intersection between cases in different DU (especially with standard ones), if it happens - set RequireQualifiedAccess attribute
Avoid functions with 4 and more parameters (I personally avoid even 3 and more)
Avoid tuple-parametrized functions when regular function can be used instead
Avoid using lists when arrays can be used instead
Avoid using async when task can be used (like when the only async model usage is Async.StartAsTask or Async.RunSynchronously)
Constructor-like initialization for properties instead of sequential assigning after creation
Warning on recursive task usage (it should suggest to rewrite to cycle)

vzarytovskii commented 1 year ago

Keep in mind that this issue is not to discuss analyzers which will be (may be) a part of sdk/fcs/compiler, etc.

T-Gro commented 1 year ago

The purpose of this is to give an overall idea of the set of tasks desired by F# programmers, in order to correctly judge the necessary "analyzing power" of the F# Analyzers SDK, keeping both performance and easy of use in mind.

From that perspective, the ideas from @Lanayx are good, as they cover a broader range of language constructs.

That being said, the main purpose of this tracking issue is to guide to an SDK design that will enable people to write such analyzers as 3prd party extensions, NOT making those analyzers as part of this repository.

BoundedChenn31 commented 1 year ago

Just to clarify, I meant those only as examples of analyzers that some people would want to port to F# SDK, hence they could be explored for SDK better designing. Sorry for bad explanation.

For example, analyzer for task and backgroundTask probably would be disabled by default and enabled through .editorconfig by configuring severity. Maybe SDK should provide API for that. But even then warning from analyzer would be useless in the context of ASP.NET Core because there is no synchronization context. So SDK should allow analyzer to find out a broader context of where it's running.

But maybe I'm missing the point of this issue again or going into too much detail :)

T-Gro commented 1 year ago

You got the point right, this is good. e.g. ability for the analyzer to check project types (or investigate other references) is an important one for the SDK design. The same for config, possibly with overrides at hierarchical levels (machine > solution > project)

nojaf commented 1 year ago

I would also like to add that existing pain points which would be solved if an F# Analyzer solution existed can, in theory, be solved today using the Ionide Analyzers SDK.

The Ionide project has no desire to compete with the official F# 9 integration. It exists as a hands-on way to experiment with analyzers right here right now. I find it immensely valuable to consume and create these analyzers as it gives you great insight into what you can get out of analyzers.

There currently are two projects that have implemented some analyzers:

That last project has implemented some ideas that were discussed in this thread (UnnamedDiscriminatedUnionFieldAnalyzer, IgnoreFunctionAnalyzer, CopyAndUpdateRecordChangesAllFieldsAnalyzer) and can be used today!

I really hope this can encourage people to discuss analyzers hands-on.

Once analyzers land in F# 9 (which again will take probably another year), the Ionide SDK can be sunset. But at the very least we can already get accustomed to having analyzers as part of our workflow.

smoothdeveloper commented 1 year ago

Constructor-like initialization for properties instead of sequential assigning after creation

@Lanayx, this could also apply to any method call that returns an object.

Some of the analyzers will require custom settings, I don't know if the C# SDK allows any of this, but it would be good for analyzers to come with a default, and through reflection / metadata and a DSL which binds to it, have the ability to configure the analyzers.

@nojaf, the effort on ionide SDK are very good, also, it may be better to consider official analyzer SDK is "cooking" but there is no commitment for the next major release; I'd prefer the stuff that ships to handle all the use cases, and come with the right implementation choices.

@vzarytovskii, in a discussion thread, I was wondering if the analyzer SDK could inspect the analyzer's used FCS surface area, and through this and hosting several versions of FCS if needed, handle the binary compatibility issue, does that sound like tractable solution? Given how the ionide SDK is laid out (with visitors and higher level APIs), I'm not sure it is feasible / meaningful though.

kevinbarabash commented 1 year ago

I would like to submit #16259: Warn about the use of non-generic collections for consideration.

smoothdeveloper commented 1 year ago

Regarding custom settings, it comes to me that an analyzer referencing another one would possibly be able to export a chunk of settings.

Say a blacklisted types analyzer would be able to import new types in the list that are exported by other analyzers referencing it.

T-Gro commented 1 year ago

@vzarytovskii, in a discussion thread, I was wondering if the analyzer SDK could inspect the analyzer's used FCS surface area, and through this and hosting several versions of FCS if needed, handle the binary compatibility issue, does that sound like tractable solution? Given how the ionide SDK is laid out (with visitors and higher level APIs), I'm not sure it is feasible / meaningful though.

Your idea would be somewhat related to having the analyzers distributed as source (or source-like), so that the compiler could investigate them. I had that idea as well, since it moves away from binary compatibility to source-code compatibility (e.g. heavy usage of DUs and pattern matching might be source-code-compatible, but not necessary binary-compatible).

It is one of the options to investigate. AS of now, I am more inclined to isolated (outside of FCS data structures) and versioned interfaces between the analyzers and the compiler, with the future compiler carrying the responsibility to bridge up to N older versions of the interfaces.

T-Gro commented 1 year ago

Regarding custom settings, it comes to me that an analyzer referencing another one would possibly be able to export a chunk of settings.

Say a blacklisted types analyzer would be able to import new types in the list that are exported by other analyzers referencing it.

Is this a shared mutable state suggestion, or not? :))

smoothdeveloper commented 1 year ago

Your idea would be somewhat related to having the analyzers distributed as source (or source-like), so that the compiler could investigate them.

The API surface analysis would work for binary shipping of analyzers in context of what came to mind when writing the above.

I think there are some complexity related to analyzer dependencies that would make shipping them as code a bit more difficult, would need to be doing like Fable, which embeds the source code in the nuget package.

AS of now, I am more inclined to isolated (outside of FCS data structures) and versioned interfaces between the analyzers and the compiler, with the future compiler carrying the responsibility to bridge up to N older versions of the interfaces.

Isolated, and basically, hosting ALL versions of FCS, with an optimization step based on the signature analysis of the analyzers (+ their dependencies...) to only instanciate the minimum set of FCS version hosts, by default.

This incurs work on the host, but comes with no cost in terms of FCS versioning (but we keep conservative with breaking change of established APIs there) from version to version and deprecating old analyzers in recent compiler.

I think the compatibility layers in FCS itself will add clutter, while having the host dealing with the subtleties and optimizing for runtime performance, would make more sense to me.

Is this a shared mutable state suggestion, or not? :))

It is a bunch of things:

analyzers exporting things
analyzers importing things exported from others
ability for analyzer to take arbitrary settings

I think there should be formal way to handle it, such as back & forth serialization to command line arguments and a text file format, a way to surface this as UI to tweak it in IDEs, and also, plain F# types (DU & records) to define those settings.

smoothdeveloper commented 1 year ago

https://github.com/fsharp/fslang-suggestions/issues/414 for [<RequireNamedArguments>].

@baronfel, could you help editing the suggestion:

replacing places showing the attribute to [<RequireNamedArgument>]
adjusting the title to "[<RequireNamedArgument>] to require the use of named arguments at callsite"
adding link to https://github.com/dotnet/runtime/issues/51451 as related

Thanks!

smoothdeveloper commented 1 year ago

T-Gro commented 1 year ago

related: fsharp/fslang-suggestions#806 & #1019 (comment)

That is an interesting one - unline all the others, it would be aimed for doing "range arithmetics", and not just operating on the typed/untyped trees.

Before I had the opinion of source code (and therefore constructs like range) being an implementation detail, but this issue link proves this is not the case - especially in an indentation-sensitive language.

dotnet / fsharp