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

Upgrade Diagnostics to JSON-RPC #9134

Closed
Simn opened this issue Feb 11, 2020 · 12 comments · Fixed by #11412
Closed

Upgrade Diagnostics to JSON-RPC #9134

Simn opened this issue Feb 11, 2020 · 12 comments · Fixed by #11412
Assignees
Labels
feature-ide IDE / Editor support
Milestone

Comments

@Simn
Copy link
Member

Simn commented Feb 11, 2020

As discussed.

@Simn Simn added the feature-ide IDE / Editor support label Feb 11, 2020
@Simn Simn added this to the Release 4.1 milestone Feb 11, 2020
@Simn Simn assigned Simn and Gama11 Feb 11, 2020
Simn added a commit that referenced this issue Feb 13, 2020
@Simn
Copy link
Member Author

Simn commented Feb 13, 2020

What method names should we use here? "display/diagnostics" for local and "project/diagnostics" for global?

@Gama11
Copy link
Member

Gama11 commented Feb 13, 2020

Hm... project/ would be new right? I see that I've used that in a TODO comment in Display.hx, but I'm not sure how much sense that makes. Haxe doesn't really have a concept of what a "project" is, right?

Do these two even need separate methods? It could be as simple as "display/diagnostics" for both, with file == null implying "global".

I also wonder if it would make sense to be able to specify a list of files in a single request for future-proofing (we might want diagnostics for all files that are currently opened in the editor, for instance). Or would that not really be much more efficient than N separate requests anyhow?

@Simn
Copy link
Member Author

Simn commented Feb 13, 2020

We could support multiple files, though I don't know in which case this would actually be useful.

The problem with omitting file is that we have some logic related to stdin consumption. Basically if you don't provide a file, the compiler tries to use the stdin input as the pseudo display file.

@Gama11
Copy link
Member

Gama11 commented Feb 13, 2020

Hm, that reminds me, we talked about being able to provide the contents for multiple files, not just the display file (since there can be multiple files with unsaved changes in an editor).

The current approach of only updating diagnostics of the currently focused file has some drawbacks. Sometimes it leads to errors from other open files sticking around when they've already been fixed. That's why we might want to update diagnostics for all open editors at once someday - at least if it's not much more expensive to do so.

@Simn
Copy link
Member Author

Simn commented Feb 13, 2020

Alright, I'll work towards supporting multiple files then.

And yes, having contents was a bit short-sighted. We could have { fileContents: Array<{ file: String, contents: String }> } in the API and then handle that in a clean way internally. I suppose we can drop stdin support from the JSON-RPC protocol entirely anyway.

@Gama11
Copy link
Member

Gama11 commented Feb 13, 2020

I guess we would have to keep contents for backwards-compatibility though, or what's the strategy there? And probably mark it as @:deprecated.

Any thoughts on the performance of requesting diagnostics for 1 file vs N files (where N is maybe 10 or so) - is that something that has gotten a lot cheaper with that recent texpr refactor? I guess we would have to measure to be sure if that's viable.

@Simn
Copy link
Member Author

Simn commented Feb 13, 2020

We would still support file and contents and just pretend that it's an element of that array if they're provided. That way we don't break anything.

1 diagnostics with N files is definitely faster than N diagnostics with 1 file due to the way diagnostics is implemented. Though this is likely something we should look into as a separate issue.

@Simn
Copy link
Member Author

Simn commented Nov 21, 2023

@kLabz I've assigned you to this because you're currently working in a very related area. Would be good to finally get this done. Ideally, I'd like to entirely remove all the old --display stuff from Haxe 5. This one shouldn't be too hard because it already generates nice JSON things.

@kLabz
Copy link
Contributor

kLabz commented Jul 2, 2024

Hm, that reminds me, we talked about being able to provide the contents for multiple files, not just the display file (since there can be multiple files with unsaved changes in an editor).

The current approach of only updating diagnostics of the currently focused file has some drawbacks. Sometimes it leads to errors from other open files sticking around when they've already been fixed. That's why we might want to update diagnostics for all open editors at once someday - at least if it's not much more expensive to do so.

This shouldn't be hard to implement, but I'm not sure how the choice to run for current file or all open files should be made.

  • Always run for all open files (with a configuration to opt out)
  • Add that possibility as opt-in
  • Keep current behavior, and add some command for running diagnostics on all open files (similar to global diagnostics)

With a note that both this new behavior and that potential new command would only be available if display/diagnostics method is available.

@Simn
Copy link
Member Author

Simn commented Jul 2, 2024

To me this is a typical "What do other languages do?"-situation.

@kLabz
Copy link
Contributor

kLabz commented Jul 2, 2024

1 diagnostics with N files is definitely faster than N diagnostics with 1 file due to the way diagnostics is implemented

This currently leads me to first solution (especially since diagnostics on N files is almost as fast as diagnostics on 1 file)

Will take a look at other languages, though I'm not sure what languages work in a similar way

@Simn
Copy link
Member Author

Simn commented Jul 2, 2024

With how interconnected compilation is in general there's not much of a difference in performance if we add more sources. Most files cannot be diagnosed independently anyway, so all this really comes down to is a glorified filter mechanism. This makes me lean towards the first solution as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-ide IDE / Editor support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants