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

Prism wrongly complaining about an unused variable #3006

Open
soundnotation opened this issue Jan 3, 2025 · 5 comments
Open

Prism wrongly complaining about an unused variable #3006

soundnotation opened this issue Jan 3, 2025 · 5 comments
Labels
bug Something isn't working vscode This pull request should be included in the VS Code extension's release notes

Comments

@soundnotation
Copy link

Description

Ruby LSP Information

VS Code Version

1.96.2

Ruby LSP Extension Version

0.8.16

Ruby LSP Server Version

0.17.2

Ruby LSP Addons

Ruby Version

3.2.2

Ruby Version Manager

rvm

Installed Extensions

Click to expand
  • alignment (0.3.0)
  • auto-add-brackets (0.12.2)
  • charactercount (0.1.0)
  • copilot (1.254.0)
  • copilot-chat (0.23.2)
  • debugpy (2024.14.0)
  • gc-excelviewer (4.2.62)
  • git-autoconfig (0.0.2)
  • gitblame (11.1.1)
  • githistory (0.6.20)
  • gitignore (0.9.0)
  • gitlens (16.1.1)
  • isort (2023.10.1)
  • java (1.38.0)
  • jupyter (2024.11.0)
  • jupyter-renderers (1.0.21)
  • mario (2.4.0)
  • markdown-all-in-one (3.6.2)
  • python (2024.22.1)
  • rails (0.22.0)
  • rails-i18n (0.8.0)
  • ruby-lsp (0.8.16)
  • sftp (1.16.3)
  • vscode-docker (1.29.3)
  • vscode-eslint (3.0.10)
  • vscode-gradle (3.16.4)
  • vscode-java-debug (0.58.1)
  • vscode-java-dependency (0.24.1)
  • vscode-java-pack (0.29.0)
  • vscode-language-pack-de (1.96.2024121109)
  • vscode-maven (0.44.0)
  • vscode-open-in-github (1.3.6)
  • vscode-pylance (2024.12.1)
  • vscode-subword-navigation (1.2.0)

Ruby LSP Settings

Click to expand
Workspace
{
  "featureFlags": {
    "all": true,
    "launcher": false
  }
}
User
{
  "enabledFeatures": {
    "codeActions": true,
    "diagnostics": true,
    "documentHighlights": true,
    "documentLink": true,
    "documentSymbols": true,
    "foldingRanges": true,
    "formatting": true,
    "hover": true,
    "inlayHint": true,
    "onTypeFormatting": true,
    "selectionRanges": true,
    "semanticHighlighting": true,
    "completion": true,
    "codeLens": true,
    "definition": true,
    "workspaceSymbol": true,
    "signatureHelp": true,
    "typeHierarchy": true
  },
  "featuresConfiguration": {},
  "addonSettings": {},
  "rubyVersionManager": {
    "identifier": "rvm"
  },
  "customRubyCommand": "",
  "formatter": "rubocop",
  "linters": null,
  "bundleGemfile": "",
  "testTimeout": 30,
  "branch": "",
  "pullDiagnosticsOn": "both",
  "useBundlerCompose": false,
  "bypassTypechecker": false,
  "rubyExecutablePath": "",
  "indexing": {},
  "erbSupport": true,
  "featureFlags": {
    "all": true,
    "launcher": false
  }
}

Reproduction steps

In a Ruby file containing an assigned and apparently unused variable, e.g.
foo = ...
I'm using this variable as a string param to binding.local_variable_get later on in the code (from a hash key), like:

{ bar: ... , foo: ... }.each do |k, v|
  binding.local_variable_get(k).do_something
  # do something with k and v
end

Prism will mark the assignment line wrongly as a warning ('assigned but unused variable - foo'). Copilot suggests me to either suppress this warning with rubocop (but rubocop has nothing to do with it - it will actually complain about Lint/RedundantCopDisableDirective: Unnecessary disabling of Lint/UselessAssignment) or use an underscore before the variable like _foo (but this will break the code later on when I'll be using k - sure, I could implement a workaround, but it will be painful to implement this in other code parts as well, and I'm pretty much sure it is already reproduced more than once).
Is there a way to disable Prism on the assignment line manually?

Code snippet or error message

  assigned but unused variable - foo Prism
@soundnotation soundnotation added bug Something isn't working vscode This pull request should be included in the VS Code extension's release notes labels Jan 3, 2025
@soundnotation soundnotation changed the title Prism falsely complaining about an unused variable Prism wrongly complaining about an unused variable Jan 3, 2025
@soundnotation
Copy link
Author

I hope this is the right place to ask. If not, please suggest me where / whom to write.

@vinistock
Copy link
Member

Thank you for the report. A few points about what you brought up:

  1. Your Ruby LSP server version is quite old (v0.17.2). We're already on v0.22.1. Please try updating and if you're having trouble, check our outdated version docs
  2. Meta-programming like the code sample you provided is often either very difficult or impossible to analyze statically, so it's expected that Prism will present you with a false positive in that case. When you're using local_variable_get, which local variables are used inside the scope can depend on runtime values, which the parser has no idea of. For example, if we expand your example to include values coming from user input or the database, then it becomes fully impossible to analyze:
# This example is impossible to analyze statically and will always raise false positives
# because the parser has no way to know which runtime values are going to be available here
key = get_desired_key_from_database

local_variable_that_is_equal_to_one_two_three = { foo_bar: ... }.find do |k, v|
  binding.local_variable_get(key) == 123
end
  1. Finally, to the last point, there's indeed no way to ignore Prism diagnostics at the moment. If Prism warns about that unused variable even when you run the code, then ideally Prism would support a way to suppress warnings. That way, suppressing a warning will make it disappear both when you run the program and in the editor. If Prism doesn't show warnings when running the code and those are editor exclusive, then we will need to add something on our side. @kddnewton do you have opinions on this one? Do you want this to be on the Prism side?

@soundnotation
Copy link
Author

Your Ruby LSP server version is quite old (v0.17.2). We're already on v0.22.1. Please try updating and if you're having trouble, check our outdated version docs

I've now updated my server version (gem) to 0.22.1 (sorry, I thought it were the latest version given that a previous bundle update didn't actually update it, but now it could update to latest, probably because I removed solargraph in between), but the issue persists.

Finally, to the last point, there's indeed no way to ignore Prism diagnostics at the moment. If Prism warns about that unused variable even when you run the code, then ideally Prism would support a way to suppress warnings. That way, suppressing a warning will make it disappear both when you run the program and in the editor. If Prism doesn't show warnings when running the code and those are editor exclusive, then we will need to add something on our side. @kddnewton do you have opinions on this one? Do you want this to be on the Prism side?

Unfortunately, the only suggestions (quick fixes) are either 'Fix using Copilot' (leads to removing the assignment, which of course will cause an error), or 'Explain with Copilot (leads to the suggestions described in my first comment, also not helping). The warning is not suppressed and since I use Windows Terminal to execute, it seems that the warning is editor exclusive.
Let me know how you want to proceed! Thanks for having a look.

@kddnewton
Copy link
Contributor

This warning is not editor-only. These warnings mirror exactly the ones output by Ruby itself. For example:

foo = 1
binding.local_variable_get(:foo)

This script will warn:

$ ruby -cw test.rb
test.rb:1: warning: assigned but unused variable - foo
Syntax OK

Note that Ruby has two levels of warnings. The first are always output, the second are output with -w is passed or $VERBOSE == true. This falls into the second category.

As you indicated, you can eliminate the warning using an _ as the first character, so:

_foo = 1
binding.local_variable_get(:_foo)

would work.

@vinistock I think it could potentially make sense in this rare case to support some kind of warning disabling feature, but it would be pretty niche to maintain, especially when there are workarounds. But if you wanted, something like # warnings: false as a pragma before this line could be detected through the "magic comments" feature of Ruby.

@vinistock
Copy link
Member

Yes, I agree that it would be niche to maintain our own syntax for disabling warnings, especially stuff coming from the parser and not a linter.

We could also allow users to configure the verbosity level, but I'm not sure how much value that brings. People would need to remember to match the verbosity level between their apps and the LSP, which feels prone to an inconsistent experience.

If Ruby had some sort of configuration file, like a ~/.rubyrc where we could read the configs and match what's going to be applied to programs, then we could guarantee better consistency. But if this is configured through $VERBOSE = true and the command line, there's little hope without introducing more settings.

Regarding the quickfixes, Copilot adds those to all diagnostics, it's not the Ruby LSP adding those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working vscode This pull request should be included in the VS Code extension's release notes
Projects
None yet
Development

No branches or pull requests

3 participants