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

[display] diagnostics miss errors from Haxe's post-processing #7931

Closed
Tracked by #11707
Gama11 opened this issue Mar 6, 2019 · 16 comments · Fixed by #11729
Closed
Tracked by #11707

[display] diagnostics miss errors from Haxe's post-processing #7931

Gama11 opened this issue Mar 6, 2019 · 16 comments · Fixed by #11729
Assignees
Labels
Milestone

Comments

@Gama11
Copy link
Member

Gama11 commented Mar 6, 2019

class Main {
	static function main() {
		var s:String;
		s += "test";
	}
}

This fails to compile with Local variable s used without being initialized, but there's no diagnostics error for it.

Seems like this is not a regression / has always been this way.

@Simn
Copy link
Member

Simn commented Mar 6, 2019

Any error from our post-processing is missed because we don't run it for diagnostics.

We can explicitly run the var init check and null-safety in diagnostics mode. They are early filters, so it should be fine.

@Simn
Copy link
Member

Simn commented Mar 6, 2019

By the way, do we have diagnostics tests now?

@Simn Simn added this to the Release 4.1 milestone Mar 6, 2019
@Gama11
Copy link
Member Author

Gama11 commented Mar 6, 2019

Yep, although just a single one that checks that there are no diagnostics. So no hasDiagnostics() helper function or anything like that yet.

b20cb70#diff-d56e6b80704188e7ea0e5d667a79876b

@Simn
Copy link
Member

Simn commented Mar 6, 2019

Not easy to fix because we don't have access to a good error function in post-processing. This is going to require some refactoring.

@Simn Simn changed the title [display] diagnostics miss "used without being initialized" errors [display] diagnostics miss errors from Haxe's post-processing Mar 6, 2019
@Gama11
Copy link
Member Author

Gama11 commented Mar 6, 2019

I assume "Type name should start with an uppercase letter" is one of these too?

@Simn
Copy link
Member

Simn commented Mar 6, 2019

No, that's a parser error. But it might be a hard error still.

@Simn
Copy link
Member

Simn commented Feb 20, 2020

I kinda want to solve this for 4.1, but it's not so easy. I think we can do something like try check_local_vars_init with Error _ (* turn into diagnostics *) in the diagnostics preparation step. That way we at least get one error per method.

However, the more general problem here is there is various post-processing that could cause errors and we don't want to run all of that for diagnostics. Unfortunately, this then means that diagnostics will never 100% match compilation errors.

@Gama11
Copy link
Member Author

Gama11 commented Feb 20, 2020

However, the more general problem here is there is various post-processing that could cause errors and we don't want to run all of that for diagnostics.

We don't? What's an example of something that would be too expensive to run?

@Simn
Copy link
Member

Simn commented Feb 20, 2020

The analyzer for sure, but this is more about the sum of the parts. Our post-processing is not designed to just detect errors, it's a huge AST transformation framework which just happens to spit out some errors in the process.

We should go through the individual steps and check which ones might report user-facing errors (not the assert false kind of stuff that comes from internal problems). Then we can maybe look into isolating these out and run them as part of diagnostics.

@Simn
Copy link
Member

Simn commented Nov 21, 2023

This has been addressed by #11220. I'll go through the filters and see that I add some tests.

@Simn
Copy link
Member

Simn commented Nov 22, 2023

Adding sane tests for this is kind of blocked by #9134.

kLabz added a commit that referenced this issue Nov 29, 2023
@kLabz kLabz closed this as completed Nov 29, 2023
@RblSb
Copy link
Member

RblSb commented Nov 29, 2023

Filters like null safety not yet here, this or #7933 should be reopened?

@Simn Simn reopened this Nov 29, 2023
0b1kn00b pushed a commit to 0b1kn00b/haxe that referenced this issue Jan 25, 2024
kLabz added a commit that referenced this issue Jul 3, 2024
kLabz added a commit that referenced this issue Jul 18, 2024
* [display] diagnostics as json rpc (Backport #11412)

* [tests] use json rpc diagnostics

* [tests] Add test for 11695

* [tests] Update diagnostics tests

* Run some filters in diagnostics (#11220)

* let's see how much breaks

* [tests] enable diagnostics tests for 11177 and 11184

* [tests] Update test for 5306

* Don't cache/run filters for find reference/implementation requests (#11226)

* Only run filters and save cache on diagnostics, not usage requests

* [tests] Update test for 11184

* disable test

* add VUsedByTyper to avoid bad unused local errors

* revert @:compilerGenerated change

---------

Co-authored-by: Rudy Ges <[email protected]>

* [display] get rid of TypeloadParse.current_stdin

* [display] fix -D display-stdin handling

* [display] generalize fileContents behavior to other json rpc display calls

* [display] fix range of pattern variables

Note: not including texprConverter changes

see 160a490
see #7282

* [tests] add test for #7282

* [tests] add test for #7931

* Remove populateCacheFromDisplay config

Legacy diagnostics = false, json rpc diagnostics = true

* [std] Diagnostics request doc

* [tests] Test Json RPC diagnostics with several open files

* [diagnostics] fix multi display files (#11722)

* [diagnostics] fix json rpc diagnostics display config

* [tests] Server tests: do not fail silently when runHaxeJsonCb errors

* [tests] add more diagnostics tests

* [display] rework multiple display files handling

* clean up a bit...

* [diagnostics] handle a.b.c.hx case, even if pointless

* [diagnostics] do not skip errors during DisplayProcessing.process_display_file

* Enable display tests again...

* [tests] fix display tests

---------

Co-authored-by: Simon Krajewski <[email protected]>
@kLabz kLabz mentioned this issue Jul 18, 2024
kLabz added a commit that referenced this issue Jul 18, 2024
* [display] diagnostics as json rpc (Backport #11412)

* [tests] use json rpc diagnostics

* [tests] Add test for 11695

* [tests] Update diagnostics tests

* Run some filters in diagnostics (#11220)

* let's see how much breaks

* [tests] enable diagnostics tests for 11177 and 11184

* [tests] Update test for 5306

* Don't cache/run filters for find reference/implementation requests (#11226)

* Only run filters and save cache on diagnostics, not usage requests

* [tests] Update test for 11184

* disable test

* add VUsedByTyper to avoid bad unused local errors

* revert @:compilerGenerated change

---------

Co-authored-by: Rudy Ges <[email protected]>

* [display] get rid of TypeloadParse.current_stdin

* [display] fix -D display-stdin handling

* [display] generalize fileContents behavior to other json rpc display calls

* [display] fix range of pattern variables

Note: not including texprConverter changes

see 160a490
see #7282

* [tests] add test for #7282

* [tests] add test for #7931

* Remove populateCacheFromDisplay config

Legacy diagnostics = false, json rpc diagnostics = true

* [std] Diagnostics request doc

* [tests] Test Json RPC diagnostics with several open files

* [diagnostics] fix multi display files (#11722)

* [diagnostics] fix json rpc diagnostics display config

* [tests] Server tests: do not fail silently when runHaxeJsonCb errors

* [tests] add more diagnostics tests

* [display] rework multiple display files handling

* clean up a bit...

* [diagnostics] handle a.b.c.hx case, even if pointless

* [diagnostics] do not skip errors during DisplayProcessing.process_display_file

* Enable display tests again...

* [tests] fix display tests

---------

Co-authored-by: Simon Krajewski <[email protected]>
@kLabz
Copy link
Contributor

kLabz commented Jul 18, 2024

Filters like null safety not yet here, this or #7933 should be reopened?

Is that still a thing ? Do you have a way to repro ?

@RblSb
Copy link
Member

RblSb commented Jul 18, 2024

I think it is, see this example, but for diagnostics check
vshaxe/vshaxe#509

@kLabz
Copy link
Contributor

kLabz commented Jul 19, 2024

Indeed, thanks!

@kLabz
Copy link
Contributor

kLabz commented Jul 19, 2024

Interestingly, it reaches the error but the reporting isn't working properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants