Store the lsp client settings in shakeExtras and create a Rule to get them #731

jneira · 2020-08-16T21:22:24Z

The motivation is to expose the client settings in shake rules, concretely my goal is to enable or disable hlint diagnostics using a client setting, changing diagnostics on the fly if the user change it in the editor
- I hope being able to configure the rules (when parsing or typechecking for example?) using the client settings could be useful in general
The config is stored in the json raw format and updated using the InitializeParams and the didChangeConfigurationParamsHandler notification
The first one is empty using vscode, it notifies a configuration change when the lsp server is started. I guess other editors will use the first one (???). I assume both should update the client settings cause the onInitialConfig and onConfigChange callbacks returns the same type (it is a type param in ghcide)
The client settings has an specific rule, which uses alwaysRerun, inspired in Typecheck entire project on Initial Load and typecheck reverse dependencies of a file on saving #688. There @pepeiborra advised it can degrade performance, but i guess there will be not much configuration changes.
With this patch a Rule that depends on GetClientSettings (for example the hlint rule that produces hlint diagnostics) is rerun on a file modification or opening a new one. Ideally i would like to trigger it within the configuration change itself (maybe restarting the shake session?)

Sorry i am missing something obvious in the ghcide or shake concepts, this is my first pr touching their internals.

pepeiborra

Looks sensible to me! Please take a look at my comments about early cutoff. Moreover, I agree that it is a good idea to restart the Shake session (with the usual setSomethingModified) whenever the config changes, please add that too.

Could we have some tests to at least check that the Shake session is restarted when the config does change?

src/Development/IDE/Core/IdeConfiguration.hs

src/Development/IDE/Core/RuleTypes.hs

src/Development/IDE/Core/Rules.hs

jneira · 2020-08-16T22:25:37Z

@pepeiborra many thanks for your comments and insights, i was a little bit lost in the ghcide codebase.
thanks @wz1000 too to point me to #688 as an example of a rule to get info stored in a Var

jneira · 2020-08-21T05:37:47Z

Could we have some tests to at least check that the Shake session is restarted when the config does change?

I'll try to add it asap. Only comment that i've cherrypicked changes here in hls and now hlint suggestions honour happily the client setting 😄

pepeiborra · 2020-09-02T18:16:36Z

needs rebase (and tests!)

jneira · 2020-09-04T13:27:26Z

rebased! that was easy, implement the tests will take me some time thought cause i am not used to ghcode test suite, any tip will be welcomed 😄

pepeiborra · 2020-09-04T14:21:46Z

The test should do something along these lines:

Open a file
Wait for progress
Change the lsp client settings
Observe the session did restart. This is the tricky bit, it might be observable by waiting for progress. I recommend uncommenting the lines 3323-3327 in test/Main.hs so that you can see the LSP traffic

jneira · 2020-09-05T12:30:08Z

Yeah, i was thinking in add a test in hls with hlint, cause there it is easy to observe how hlints diagnostics come and go.

pepeiborra · 2020-09-05T14:58:48Z

I think we want to have a test here as well please. You can send custom messages to the lsp client if necessary, see how cradle loading is tested

jneira · 2020-09-08T08:50:36Z

/azp run

azure-pipelines · 2020-09-08T08:50:42Z

No pipelines are associated with this pull request.

jneira · 2020-09-16T19:50:02Z

I cant make progress with the tests cause the test suite hangs in windows. hls one too so i suspect some change in lsp-test around the shutdown of the process is involved. 😟

jneira · 2020-09-19T10:16:55Z

After fixing the hang of the test suite in windows and add some logging notifications in the Shake module i have been able to add the test cases.

wz1000 · 2020-09-20T16:20:25Z

/azp run

azure-pipelines · 2020-09-20T16:20:31Z

Pull request contains merge conflicts.

wz1000 · 2020-09-20T16:20:36Z

rebase needed

jneira · 2020-09-21T09:42:34Z

There is a failing test for ghc-8.6 and 8.10 (but no for 8.8 🤔 )

2020-09-20T21:57:49.0532485Z   Interface loading tests
2020-09-20T21:57:49.2935216Z     iface-error-test-1:                                                                                   FAIL
2020-09-20T21:57:49.2959459Z       Exception: NamedParserException "NotificationMessage ServerMethod PublishDiagnosticsParams" (NamedParserException "NotificationMessage ServerMethod PublishDiagnosticsParams" (NamedParserException "NotificationMessage ServerMethod PublishDiagnosticsParams" (NamedParserException"
.............. (NamedParserException "Response for id: IdInt 1" (NamedParserException "NotificationMessage ServerMethod (ProgressParams WorkDoneProgressBeginParams)" (BothFailed (Unexpected "ConduitParser.empty") (Unexpected "ConduitParser.empty"))))))))))))))))))))))))))))) (Unexpected "ConduitParser.empty")))))))))))))))))))))))))))))

I guess the new log notifications for shake session broke it. 😟

pepeiborra

There is a failing test for ghc-8.6 and 8.10 (but no for 8.8 🤔 )

2020-09-20T21:57:49.0532485Z   Interface loading tests
2020-09-20T21:57:49.2935216Z     iface-error-test-1:                                                                                   FAIL
2020-09-20T21:57:49.2959459Z       Exception: NamedParserException "NotificationMessage ServerMethod PublishDiagnosticsParams" (NamedParserException "NotificationMessage ServerMethod PublishDiagnosticsParams" (NamedParserException "NotificationMessage ServerMethod PublishDiagnosticsParams" (NamedParserException"
.............. (NamedParserException "Response for id: IdInt 1" (NamedParserException "NotificationMessage ServerMethod (ProgressParams WorkDoneProgressBeginParams)" (BothFailed (Unexpected "ConduitParser.empty") (Unexpected "ConduitParser.empty"))))))))))))))))))))))))))))) (Unexpected "ConduitParser.empty")))))))))))))))))))))))))))))

I guess the new log notifications for shake session broke it. 😟

You can probably simplify the failing test as below, which might help it pass:

    res <- skipManyTill anyMessage $ responseForId li

src/Development/IDE/Core/Shake.hs

@pepeiborra

Following @pepeiborra advise

jneira · 2020-09-22T14:17:05Z

Really tired of fpcomplete timeouts 😢

jneira · 2020-09-22T14:17:14Z

/azp run

azure-pipelines · 2020-09-22T14:17:25Z

Azure Pipelines successfully started running 1 pipeline(s).

jneira · 2020-09-22T19:04:47Z

/azp run

azure-pipelines · 2020-09-22T19:04:58Z

Azure Pipelines successfully started running 1 pipeline(s).

jneira · 2020-09-23T05:31:40Z

/azp run

azure-pipelines · 2020-09-23T05:31:53Z

Azure Pipelines successfully started running 1 pipeline(s).

jneira · 2020-09-23T05:32:45Z

🤞

jneira · 2020-09-23T08:44:56Z

/azp run

azure-pipelines · 2020-09-23T08:45:08Z

Azure Pipelines successfully started running 1 pipeline(s).

jneira · 2020-09-23T11:38:55Z

finally ci is green! @pepeiborra i think last requested changes are done, thanks for your patience 😄

pepeiborra · 2020-09-23T19:53:06Z

Eureka!

@pepeiborra

… them (haskell#731) * Store client settings in ide state * Log ide config registered in initHandler * Use a Maybe aware updater function * Create a Rule to get client settings * Create a specific getter for client settings * Trim trailing whitespace * Use modifyVar to avoid race conditions * Add comment to GetClientSettings * Use defineEarlyCutOffNoFile for GetClientSettings * Restart shake on config changed * Use Hashed for clientSettings * Send log notifications to client about session * Show test output directly * Add tests over client settings * Apply hlint hints * Simplify iface test to make it more robust Following @pepeiborra advise * Send session notifications only in test mode * Retry bench execution

@pepeiborra

… them (haskell/ghcide#731) * Store client settings in ide state * Log ide config registered in initHandler * Use a Maybe aware updater function * Create a Rule to get client settings * Create a specific getter for client settings * Trim trailing whitespace * Use modifyVar to avoid race conditions * Add comment to GetClientSettings * Use defineEarlyCutOffNoFile for GetClientSettings * Restart shake on config changed * Use Hashed for clientSettings * Send log notifications to client about session * Show test output directly * Add tests over client settings * Apply hlint hints * Simplify iface test to make it more robust Following @pepeiborra advise * Send session notifications only in test mode * Retry bench execution

@pepeiborra

… them (haskell/ghcide#731) * Store client settings in ide state * Log ide config registered in initHandler * Use a Maybe aware updater function * Create a Rule to get client settings * Create a specific getter for client settings * Trim trailing whitespace * Use modifyVar to avoid race conditions * Add comment to GetClientSettings * Use defineEarlyCutOffNoFile for GetClientSettings * Restart shake on config changed * Use Hashed for clientSettings * Send log notifications to client about session * Show test output directly * Add tests over client settings * Apply hlint hints * Simplify iface test to make it more robust Following @pepeiborra advise * Send session notifications only in test mode * Retry bench execution

@pepeiborra

… them (haskell/ghcide#731) * Store client settings in ide state * Log ide config registered in initHandler * Use a Maybe aware updater function * Create a Rule to get client settings * Create a specific getter for client settings * Trim trailing whitespace * Use modifyVar to avoid race conditions * Add comment to GetClientSettings * Use defineEarlyCutOffNoFile for GetClientSettings * Restart shake on config changed * Use Hashed for clientSettings * Send log notifications to client about session * Show test output directly * Add tests over client settings * Apply hlint hints * Simplify iface test to make it more robust Following @pepeiborra advise * Send session notifications only in test mode * Retry bench execution

jneira changed the title ~~Store the lsp client settings in shakeExtras~~ Store the lsp client settings in shakeExtrasand create a Rule to get them Aug 16, 2020

pepeiborra suggested changes Aug 16, 2020

View reviewed changes

src/Development/IDE/Core/IdeConfiguration.hs Outdated Show resolved Hide resolved

src/Development/IDE/Core/RuleTypes.hs Outdated Show resolved Hide resolved

src/Development/IDE/Core/Rules.hs Show resolved Hide resolved

jneira mentioned this pull request Aug 16, 2020

Hlint plugin using ghc-lib haskell/haskell-language-server#166

Merged

7 tasks

jneira changed the title ~~Store the lsp client settings in shakeExtrasand create a Rule to get them~~ Store the lsp client settings in shakeExtras and create a Rule to get them Aug 16, 2020

jneira requested a review from pepeiborra September 19, 2020 10:17

jneira added 11 commits September 20, 2020 21:49

Store client settings in ide state

a9ecc1b

Log ide config registered in initHandler

27344a1

Use a Maybe aware updater function

1f33d5d

Create a Rule to get client settings

e877cdf

Create a specific getter for client settings

3f1cf81

Trim trailing whitespace

d58fd28

Use modifyVar to avoid race conditions

3ebe718

Add comment to GetClientSettings

62e2229

Use defineEarlyCutOffNoFile for GetClientSettings

0c046fe

Restart shake on config changed

b6c51aa

Use Hashed for clientSettings

fb93dff

jneira added 4 commits September 20, 2020 23:19

Send log notifications to client about session

5fb47bb

Show test output directly

d7215bd

Add tests over client settings

46f66c0

Apply hlint hints

9035559

pepeiborra reviewed Sep 21, 2020

View reviewed changes

src/Development/IDE/Core/Shake.hs Outdated Show resolved Hide resolved

src/Development/IDE/Core/Shake.hs Outdated Show resolved Hide resolved

src/Development/IDE/Core/Shake.hs Outdated Show resolved Hide resolved

jneira added 2 commits September 22, 2020 12:56

Simplify iface test to make it more robust

f142fd1

Following @pepeiborra advise

Send session notifications only in test mode

1486819

Retry bench execution

b2617fe

jneira requested a review from pepeiborra September 23, 2020 11:39

pepeiborra approved these changes Sep 23, 2020

View reviewed changes

pepeiborra merged commit c361a26 into haskell:master Sep 23, 2020

jneira deleted the client-settings-da branch October 6, 2020 05:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store the lsp client settings in shakeExtras and create a Rule to get them #731

Store the lsp client settings in shakeExtras and create a Rule to get them #731

jneira commented Aug 16, 2020

pepeiborra left a comment

jneira commented Aug 16, 2020 •

edited

Loading

jneira commented Aug 21, 2020

pepeiborra commented Sep 2, 2020

jneira commented Sep 4, 2020

pepeiborra commented Sep 4, 2020

jneira commented Sep 5, 2020

pepeiborra commented Sep 5, 2020

jneira commented Sep 8, 2020

azure-pipelines bot commented Sep 8, 2020

jneira commented Sep 16, 2020

jneira commented Sep 19, 2020

wz1000 commented Sep 20, 2020

azure-pipelines bot commented Sep 20, 2020

wz1000 commented Sep 20, 2020

jneira commented Sep 21, 2020

pepeiborra left a comment

jneira commented Sep 22, 2020

jneira commented Sep 22, 2020

azure-pipelines bot commented Sep 22, 2020

jneira commented Sep 22, 2020

azure-pipelines bot commented Sep 22, 2020

jneira commented Sep 23, 2020

azure-pipelines bot commented Sep 23, 2020

jneira commented Sep 23, 2020

jneira commented Sep 23, 2020

azure-pipelines bot commented Sep 23, 2020

jneira commented Sep 23, 2020

pepeiborra commented Sep 23, 2020

Store the lsp client settings in shakeExtras and create a Rule to get them #731

Store the lsp client settings in shakeExtras and create a Rule to get them #731

Conversation

jneira commented Aug 16, 2020

pepeiborra left a comment

Choose a reason for hiding this comment

jneira commented Aug 16, 2020 • edited Loading

jneira commented Aug 21, 2020

pepeiborra commented Sep 2, 2020

jneira commented Sep 4, 2020

pepeiborra commented Sep 4, 2020

jneira commented Sep 5, 2020

pepeiborra commented Sep 5, 2020

jneira commented Sep 8, 2020

azure-pipelines bot commented Sep 8, 2020

jneira commented Sep 16, 2020

jneira commented Sep 19, 2020

wz1000 commented Sep 20, 2020

azure-pipelines bot commented Sep 20, 2020

wz1000 commented Sep 20, 2020

jneira commented Sep 21, 2020

pepeiborra left a comment

Choose a reason for hiding this comment

jneira commented Sep 22, 2020

jneira commented Sep 22, 2020

azure-pipelines bot commented Sep 22, 2020

jneira commented Sep 22, 2020

azure-pipelines bot commented Sep 22, 2020

jneira commented Sep 23, 2020

azure-pipelines bot commented Sep 23, 2020

jneira commented Sep 23, 2020

jneira commented Sep 23, 2020

azure-pipelines bot commented Sep 23, 2020

jneira commented Sep 23, 2020

pepeiborra commented Sep 23, 2020

jneira commented Aug 16, 2020 •

edited

Loading