Skip to content

Don't suggest -Wno-deferred-out-of-scope-variables as a possible action. #4440

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

Closed
jeukshi opened this issue Oct 28, 2024 · 6 comments · Fixed by #4441
Closed

Don't suggest -Wno-deferred-out-of-scope-variables as a possible action. #4440

jeukshi opened this issue Oct 28, 2024 · 6 comments · Fixed by #4441

Comments

@jeukshi
Copy link
Contributor

jeukshi commented Oct 28, 2024

Is your enhancement request related to a problem? Please describe.

With code

f = let x = Doesn'tExist
     in undefined

when I execute code action on Doesn'tExist, HLS (haskell-language-server version: 2.9.0.0 (GHC: 9.6.6)) adds {-# OPTIONS_GHC -Wno-deferred-out-of-scope-variables #-} on the top of my file. It does this, I believe, because this is the only code action available. I don't think this is the solution that user wants in most cases. At least in Emacs it is easy to miss that HLS did that (if only one action is available, it is executed), leading to confusion on as to why, suddenly, HLS "doesn't work". It works, of course, because with that flag there is no longer a compile time error.

Describe the solution you'd like

Don't suggest {-# OPTIONS_GHC -Wno-deferred-out-of-scope-variables #-}.

Describe alternatives you've considered

Restarting HLS... a lot :) I don't think there is an alternative.

@fendor
Copy link
Collaborator

fendor commented Oct 29, 2024

Hi, thank you for your bug report!

Does emacs run the code action for a particular location when there is only one code action? That feels like rather bad behaviour to me. It can easily happen that the only available code action is not something you want to execute, imo. Are you sure that's emacs behviour?

Why do you need to restart HLS to undo the code action? Shouldn't undo make HLS display the error again?

@jeukshi
Copy link
Contributor Author

jeukshi commented Oct 29, 2024

Are you sure that's emacs behviour?

Doom Emacs, at least, yes. It is... questionable UX, I agree.

If it is not a constructor, like 'func = doesn'tExist', then I get 3 actions I can choose:

  • Define doesn'tExist :: _
  • Add argument 'doesn'tExist' to a function
  • Disable "deferred-out-of-scope-variables"

Why do you need to restart HLS to undo the code action?

I know it should be an error there, but there is none reported by HLS (it got deferred by the flag and I didn't notice). So I try to get it back. Imagine copy-pasting some code, running add import on a bunch of variables and this guy gets inserted along the way, on the top of the file. I spend some time figuring it out, twice now.

But ignoring Emacs behavior, let me elaborate.

Is Disable "deferred-out-of-scope-variables" really an action that HLS should offer? It seems to me this flag is "know what you are doing"/"you should know when you need it" type of deal. Rather niche at that. Not to mention, it does not apply to a variable, but a whole file. And, worse, it doesn't even fix the issue, it just changes Haskell into Python. All this in the name of helping with... a typo. I would rather have no action here and fix my error by hand, properly.

@fendor
Copy link
Collaborator

fendor commented Oct 29, 2024

Related issue #2032, seems like we have some prior art of special casing some warnings. See PR #2061 for the fix.
I feel like this approach of blacklisting flags is not the most maintainable, but I agree this flag is extremely rarely what you want.

Fixes welcome!

Either way, I think doom emacs's behaviour is broken, too.

@jeukshi
Copy link
Contributor Author

jeukshi commented Oct 29, 2024

Seeing that PR I think you already did 90% of the work by just finding it! I'll then do my part and nuke that thing.

As for Emacs, well, it's Emacs, can be configured, probably.

jeukshi added a commit to jeukshi/haskell-language-server that referenced this issue Oct 29, 2024
jeukshi added a commit to jeukshi/haskell-language-server that referenced this issue Oct 29, 2024
Fixes haskell#4440

Fixes test for disabling deferred-type-errors.
@Bodigrim
Copy link
Contributor

Bodigrim commented Nov 2, 2024

Could HLS set -Wno-deferred-out-of-scope-variables under the hood to make further progress with compilation and report more diagnostics?

@fendor
Copy link
Collaborator

fendor commented Nov 3, 2024

@fendor fendor closed this as completed in 96bea00 Nov 3, 2024
soulomoon pushed a commit to soulomoon/haskell-language-server that referenced this issue Nov 4, 2024
Fixes haskell#4440

Fixes test for disabling deferred-type-errors.
fendor pushed a commit to fendor/haskell-language-server that referenced this issue Dec 23, 2024
Fixes haskell#4440

Fixes test for disabling deferred-type-errors.
fendor added a commit that referenced this issue Jan 4, 2025
* Change FileDiagnostic type synonym to a datatype

* Make `ideErrorWithSource` produce FileDiagnostic by adding filepath arg

* Supply structured error wherever we easily can - TODOs for hard parts

We're leaving the TODOs for either later in this PR or in another PR

* Fix UnitTests for new FileDiagnostic struct

* Remove explicit uses of FileDiagnostic, add codes to LSP diagnostics

* Add field for expected error codes in ghcide tests

* Expect GHC-83865 for "type error" test - basic test

* Return structured warnings in TcModuleResult by copying from Driver

* Store FileDiagnostic instead of LSP Diagnostic in Shake store

* Add expected error codes for diagnostics that have them

* Dispatch TODOs, amend remaining TODOs as future work

* Add scary comments all over copied code in Compat.Driver

* Update all remaining diagnostics that could use an expected error code

* Add _code to pretty printing for FileDiagnostic

* Use case instead of `maybe` for StructuredMessage match

* Use CPP to prevent setting _code before structured errors

* Swap modifier for lenses, document StructuredMessage type

* Add link to Issue & MR to Compat.Driver

* Drop attachReason logic from withWarnings, technically incorrect

* Revert "Drop attachReason logic", needed by pragmas-plugin

This reverts commit 4fed987.

* Fix plugins where necessary for new diagnostic structure

* Fix build issues with other tests from `expectDiagnostics`

* Improve comment on metadata fdStructuredMessage in FileDiagnostic

* Add note to withWarnings explaining the current state of things

* Attach reasons into data field of LSP Diagnostic instead of code field

Had to move `attachReason` between modules to achieve this, which is
fine because it was never exported from its own module.

* Fix up mistakes from merge, TODO fix merge issues for 9.3.0

* Set CodeDescription from HaskellErrorIndex when available

* Remove debugging print, fix expectation for preprocessor tests

* Fix CPP for using Show instance on DiagnosticCode

* Remove diagFromErrMsgs for GHC version < 9.6.1 using CPP

* CPP fix

* More stylish-haskell, more CPP fix

* Fix all stylish-haskell errors triggering

* Fix more CPP

* Only override the LSP diagnostic code when not already set

* Fixes for stylish-haskell

stylish-haskell does not handle CPP pragmas very well, is this a
regression?

* Qualify s, t for FuzzySearch

* Ignore use of unsafePerformIO in FuzzySearch

* Properly split GHC.Types.Error import in Diagnostics for stylish-haskell

* Force type signature of annotation on FuzzySearch.dictionary

* DRY up definition of closure_errs

From review #4311 (comment)

* Remove unused imports

* Post-rebase fixes

* stylish-haskell formatting

* Fix issue with GHC 9.4

* Please stylish-haskell

* Ignore error codes when testing GHC 9.4

* Workaround darwin GHC bug in hls-hlint-plugin

* Put the workaround in the right place

* Revert "Set CodeDescription from HaskellErrorIndex when available"

This reverts commit 14d6697.

* Resolve fendor's feedback

* Apply stylish-haskell formatting

* Apply more stylish-haskell formatting

* Resolve some of soulomoon's feedback

* Fix small issues

* Remove unused imports

* Remove StructuredDiagnostic

* Revert "Remove StructuredDiagnostic"

This reverts commit 0776c65.

* Remove the unused parameter from 'ideErrorText'

* Add documentation to diagnostic helpers

* Add action to query active diagnostics for a given Range

Implement 'rangesOverlap' function which checks whether two 'Range's
overlap in any way.
Implement two new plugin utility functions which allow to conveniently
get all currently displayed diagnostics for a given 'Range'.

* Use lens for updating Diagnostic

* Add GHC Structured Error compatibility module

Add compatibility module for GHC's structured error messages.
Introduce 'Prism's and 'Lens's to easily access nested structures.
Expand documentation for 'StructuredMessage'

* Remove unused imports

* Don't suggest -Wno-deferred-out-of-scope-variables (#4441)

Fixes #4440

Fixes test for disabling deferred-type-errors.

* Build HLS with GHC 9.8.3 (#4444)

* ci(mergify): upgrade configuration to current format (#4454)

Co-authored-by: Mergify <37929162+mergify[bot]@users.noreply.github.com>

* More tests and better docs for cabal-add (#4455)

* new tests

* change codeAction title

* more tests and docs

---------

Co-authored-by: fendor <fendor@users.noreply.github.com>

* Fix compatibility with GHC 9.4 and rename function

* Use GHC Note syntax and reference Note in docs

Allows HLS to 'Goto Definition' for Note references.

* Add doc comment for 'tmrWarnings'

* Push CPP statements to compatibility module

* Fix formatting in Development.IDE.GHC.Compat.Error

---------

Co-authored-by: Dylan Thinnes <dylan.thinnes@protonmail.com>
Co-authored-by: soulomoon <fwy996602672@gmail.com>
Co-authored-by: Fendor <fendor@posteo.de>
Co-authored-by: jeukshi <orlowd+gh@gmail.com>
Co-authored-by: fendor <fendor@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Georgii Gerasev <54953043+VenInf@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants