[ty] Configure check mode for all projects#23121
Conversation
BurntSushi
requested review from
Gankra,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
BurntSushi
requested review from
dhruvmanila
and removed request for
Gankra,
MichaReiser,
carljm,
dcreager and
sharkdp
|
The test failure is real here I think. It was previously not setting the diagnostic mode, which in turn defaulted to |
Previously, we were only setting the check mode on a project if the diagnostic mode was `workspace`. While the diagnostic mode defaults to `OpenFilesOnly`, the _check mode_ on a ty project defaults to `AllFiles` (i.e., workspace). The result is that projects were always configured in `AllFiles` check mode, making it impossible to set the check mode to `OpenFilesOnly`. Confusingly, this is _distinct_ from the global settings diagnostic mode, which could actually be different thant he check mode on the individual projects! This did cause one test failure: an end-to-end test checking that changing the language of a file resulted in the LSP picking up that change. e.g., Going from Python to non-Python should result in no diagnostics. This was added in #21867, but I believe the change was incomplete: when a `didOpen` notification is received, we were unconditionally adding the file to the project state without checking its extension. The test still "worked" precisely because of the `AllFiles` vs `OpenFilesOnly` confusion. That is, the server defaulted to `OpenFilesOnly`, but the project defaulted to `AllFiles`. This in turn made it so the file was an "open file" and not actually picked up from a directory scan. That in turn led to `should_check_file` returning false, since the `AllFiles` mode explicitly ignores "open files." Ref astral-sh/ty#2616
BurntSushi
force-pushed
the
ag/fix-check-mode
branch
from
6c25981 to
07f565f
Compare
| }; | ||
| server.send_notification::<DidOpenTextDocument>(params); | ||
| let _close_diagnostics = server.await_notification::<PublishDiagnostics>(); | ||
|
|
There was a problem hiding this comment.
I'm not sure if it was intentional to have the diagnostics send back twice, but this is no longer the case. It was happening previously because:
- Closing the document (above) would result in clearing diagnostics only when
DocumentHandle::closereturnstrue. And that would return true becausesession.global_settings().diagnostic_mode().is_open_files_only()returnedtrue. Because the session global settings usedOpenFilesOnlyby default. - Re-opening the document (with the language ID changed) caused
publish_diagnostics_if_neededinside of theDidOpenTextDocumentHandlerhandler to run. This would eventually doProject::check_fileand that would callshould_check_file. And that would returnfalsebecause the projects have been defaulting toAllFilesas a check mode by default, and the file in question is only in the "open file" set. (Which is perhaps a bug on its own, but I digress.)
The end result is that the test got no diagnostics which was what was expected, but it got for a reason other than respecting the change in language ID. Once I fixed the AllFiles/OpenFilesOnly problem, this test started failing because we would still send back diagnostics for the plain text file.
I ended up fixing this by checking the language ID of the file in the DidOpenTextDocumentHandler handler. If it's Other, then we ignore it, hopefully mirroring what we do in ty_project/src/walk.rs.
dhruvmanila
left a comment
dhruvmanila
left a comment
There was a problem hiding this comment.
Uff, this seemed like very confusing to debug, thanks for tracking this down!
| let document = session.open_text_document( | ||
| TextDocument::new(uri, text, version).with_language_id(&language_id), | ||
| ); | ||
| let text_doc = TextDocument::new(uri, text, version).with_language_id(&language_id); | ||
| if matches!(text_doc.language_id(), Some(LanguageId::Other)) { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let document = session.open_text_document(text_doc); |
There was a problem hiding this comment.
I think this sounds correct but it would be useful to have a E2E test case for this scenario if it doesn't already exists. What do you think?
There was a problem hiding this comment.
I think this is what the changing_language_of_file_without_extension test covers. That the test was previously passing was a mirage. :-) Once I untangled the AllFiles/OpenFilesOnly stuff, it started failing. Adding this was required to make it pass because otherwise we treat the open file as Python code.
3 participants
Previously, we were only setting the check mode on a project if the
diagnostic mode was
workspace. While the diagnostic mode defaults toOpenFilesOnly, the check mode on a ty project defaults toAllFiles(i.e., workspace).
The result is that projects were always configured in
AllFilescheckmode, making it impossible to set the check mode to
OpenFilesOnly.Confusingly, this is distinct from the global settings diagnostic
mode, which could actually be different thant he check mode on the
individual projects!
This did cause one test failure: an end-to-end test checking that
changing the language of a file resulted in the LSP picking up that
change. e.g., Going from Python to non-Python should result in no
diagnostics. This was added in #21867, but I believe the change was
incomplete: when a
didOpennotification is received, we wereunconditionally adding the file to the project state without checking
its extension. The test still "worked" precisely because of the
AllFilesvsOpenFilesOnlyconfusion. That is, the server defaultedto
OpenFilesOnly, but the project defaulted toAllFiles. This inturn made it so the file was an "open file" and not actually picked up
from a directory scan. That in turn led to
should_check_filereturningfalse, since the
AllFilesmode explicitly ignores "open files."Ref astral-sh/ty#2616