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

Add analyzers from FSharp.Formatting PR #85

Merged
merged 12 commits into from
Mar 29, 2024
Merged

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Mar 15, 2024

In fsprojects/FSharp.Formatting#906, @Thorium made some good suggestions to improve the performance of the code.

Some of these checks are interesting to turn into an analyzer I think.
I will try and add some analyzers in this PR.

  • HeadConsEmptyListPatternAnalyzer
  • list = [] analyzer (should be List.empty)
  • Propose [<return: Struct>] on (partial) active pattern.
  • List.map piped to List.map call.
  • a = null, should be isNull a
  • a <> null should be not (isNull a)
  • Add [<Struct>] to DU when it makes sense. (For example each DU case does not have any fields)

@Thorium
Copy link

Thorium commented Mar 15, 2024

I have more suggestions if you run out :-D

  • .ContainsKey key followed by .[key] should be TryGetValue
  • ConcurrentDictionaries shouldn't be called with Add or .[x] but rather via GetOrAdd or AddOrUpdate
  • Seq.length > 0 and (x:seq).Length > 0 should be replaced with Seq.isEmpty
  • If you have large list/array/seq of items and you do .Contains in a loop, consider option to change your list to Set
  • Don't do new Regex.Match in a loop: rather let myRegex = Regex("...") and then loop your myRegex.Match
  • Also always try to close your regex, to make it “StartsWith” or “EndsWith” and not “Contains”
  • Don't do DateTime.UtcNow.Date, it points to a random date that is not what you wanted.
  • If you ever do TransactionScope remember to set TransactionScopeAsyncFlowOption to 1, as modern software is asynchronous.
  • Besides of this I did 10 years ago a list that I hoped would be "functional refactorings" in a "what is ReSharper to C#" spirit: https://www.slideshare.net/thorium/possible-fsharp-refactorings-could-be

@nojaf
Copy link
Contributor Author

nojaf commented Mar 15, 2024

I have more suggestions if you run out :-D

Thanks, that is good input. Realistically speaking I'll probably not get to those.
My time for this is limited.

@Numpsy
Copy link
Contributor

Numpsy commented Mar 15, 2024

I'm sure there are plenty of possible ideas for analyzers about (be it borrowing more rules from FSharpLint, or looking at things like NetAnalyzers and Roslynator, or more F# specific stuff) - is it worth creating a more general list of suggestions somewhere to track ideas?

@nojaf
Copy link
Contributor Author

nojaf commented Mar 15, 2024

is it worth creating a more general list of suggestions somewhere to track ideas?

Not to throw shade, but who's actually gonna make it happen, right? 😉 Just forecasting the vibes, not trying to be harsh.

@Thorium
Copy link

Thorium commented Mar 15, 2024

Something that ionide-analysers could in theory do, and not so much of others (just because they are not F# IDE-plugins), would be these 2 way conversions of "change A to B" and "change B to A" and leave final decision for user to decide. Because that would help refactorings.

@nojaf
Copy link
Contributor Author

nojaf commented Mar 22, 2024

@dawedawe feel free to already start nitpicking if you like.

Copy link
Contributor

@dawedawe dawedawe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very solid work!

docs/performance/008.md Outdated Show resolved Hide resolved
docs/performance/010.md Outdated Show resolved Hide resolved
@nojaf nojaf marked this pull request as ready for review March 24, 2024 09:04
@nojaf nojaf requested a review from dawedawe March 24, 2024 09:09
@Numpsy
Copy link
Contributor

Numpsy commented Mar 25, 2024

A question about the Add [<Struct>] to DU when it makes sense. suggestion/fix:

Say I have a DU in one of these forms:

type SomeType =
| Success
| SomeError of string
| SomeOtherError of string

type SomeType =
| Success
| SomeError of error: string
| SomeOtherError of error: string

I get the 'make it a struct' suggestion and quick fix:
image
But if I action the quick fix, I get:
image

Maybe making sure the cases have unique names is a good suggestion anyway, but fwiw - should the quick fix make a change that then errors?

@nojaf
Copy link
Contributor Author

nojaf commented Mar 28, 2024

A question about the Add [] to DU when it makes sense. suggestion/fix:

I've noticed the issue as well. Ideally, the quick fix should address this. I'm inclined to accept the quick fix as it stands. While it's not ideal, anyone interested can further refine it with subsequent PRs. I feel this option might be the better choice compared to turning off the code fix and nobody ever touches it again.

This PR's work is supported by the Amplifying F# Open Collective (more details to be shared publicly later this week). We allocated one working day for this task, and we've now surpassed that limit. I'll be shifting my focus to other work that supports my livelihood.

Copy link
Contributor

@dawedawe dawedawe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. You really squeezed the funding to the max. Thanks!
The known limitations should be handled in another round.

@nojaf nojaf merged commit 6e3a03e into ionide:main Mar 29, 2024
5 checks passed
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