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

Improvements/Fixes for Unused Declarations #998

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

Booksbaum
Copy link
Contributor

@Booksbaum Booksbaum commented Aug 22, 2022

Change: Get Unused Declarations from FCS

  • -> remove custom UnusedDeclarationAnalyzer

Enhancement: mark more elements (like functions & members) as unused


Enhancement/Change: Unused Diagnostic now always contains Code FSAC0003

  • prev: only some unused diags contained FSAC0003.
    Code was used to differentiate between this value (-> only replace with _ CodeFix) and other values (-> both Replace & prefix CodeFixes). That's now determined inside RenameUnusedValue CodeFix -> all diags can be FSAC0003
  • As a result: NotificationEvent.UnusedDeclarations(file, decls) -> decls is now just range[] (prev: (range*bool)[])

Fixes for RenameUnusedValue CodeFix:

  • Fix: name in backticks triggers "Prefix with _" CodeFix. But let _``hello world`` = ... is invalid.
    -> Prefix CodeFix doesn't trigger any more for backticks. Replace with _ still does.
  • Fix_ish: private variable triggers "Replace with _" CodeFix. But let private _ = ... is invalid (private is part of named SynPat, not part of let binding or outer SynPat)
    • Issue: SynPat.Named range doesn't include access modifier. And SynAccess doesn't include a range. (At least in FCS version currently used in FSAC. Range was introduced in Add range of access to SyntaxTree. dotnet/fsharp#13304 )
    • -> Instead of replacing the modifier too, I just disable the Replace CodeFix for cases with access modifier.
      Once there is a FCS with ranges for SynAccess it should be trivial to enable Replace CodeFix again (-> see code and corresponding tests)

  • Note: Prefix/Replace with _ CodeFix still only triggers for unused value bindings, but not functions or members.
    I don't think you ever want a _-prefixed function or member, but instead should either remove or make it public -> no prefix CodeFix (and replace with _ is invalid for functions/members)

Remove custom `UnusedDeclarationAnalyzer`

Change: Unused Diags now always contain Code `FSAC0003`

Fix: don't trigger Prefix with `_` CodeFix for backticks
Fix: don't replace private variable with `_`
@baronfel
Copy link
Contributor

awesome

You note that the SynAccess range isn't present in our FCS version - is it in FCS 41.0.5 (the latest release)? If so we should update that sometime this week, because proj-info just updated to it and I'm planning on updating the proj-info dependency in this repo.

@Booksbaum
Copy link
Contributor Author

FSAC is already at 41.0.5

I think SynAccess.range will be included in the next major FCS version (42?) (including other AST changed like necessary to remove ServiceParseTreeWalk again)

@baronfel baronfel merged commit 680a95b into ionide:main Aug 22, 2022
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.

Unused marking missing for private, unused functions
2 participants