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

SemanticDB diagnostics missing #17535

Closed
bjaglin opened this issue May 18, 2023 · 9 comments · Fixed by #17835
Closed

SemanticDB diagnostics missing #17535

bjaglin opened this issue May 18, 2023 · 9 comments · Fixed by #17835

Comments

@bjaglin
Copy link

bjaglin commented May 18, 2023

Compiler version

3.3.0-RC6 (but I believe this is not a regression, and is reproducible for any 3.x)

Minimized code

//> using scala 3.3.0-RC6
//> using options -Wunused:all -Ysemanticdb

import java.util.List

object Test

Output

➜  metap Test.scala.semanticdb
Test.scala
--------------

Summary:
Schema => SemanticDB v4
Uri => tmp/Test.scala
Text => empty
Language => Scala
Symbols => 1 entries
Occurrences => 4 entries

Symbols:
_empty_/Test. => final object Test extends Object { self: Test.type => +1 decls }

Occurrences:
[3:7..3:11) => java/
[3:12..3:16) => java/util/
[3:17..3:21) => java/util/List#
[5:7..5:11) <= _empty_/Test.

Expected

 ➜  metap Test.scala.semanticdb
 Test.scala
 --------------

 Summary:
 Schema => SemanticDB v4
 Uri => tmp/Test.scala
 Text => empty
 Language => Scala
 Symbols => 1 entries
 Occurrences => 4 entries

 Symbols:
 _empty_/Test. => final object Test extends Object { self: Test.type => +1 decls }

 Occurrences:
 [3:7..3:11) => java/
 [3:12..3:16) => java/util/
 [3:17..3:21) => java/util/List#
 [5:7..5:11) <= _empty_/Test.

+Diagnostics:
+[3:17..3:21) [warning] Unused import
@bjaglin bjaglin added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels May 18, 2023
@tgodzik
Copy link
Contributor

tgodzik commented May 19, 2023

I created very quick try, but it seems we are missing the warnings. Not sure how to extract the diagnostics from context.

Tried blindly to use ctx.reproter.pendingMessages but trying it out in a local project didn't yield any success.

https://github.com/lampepfl/dotty/compare/main...tgodzik:dotty:diags-semanticdb?expand=1

Anyone can help out here?

@tgodzik
Copy link
Contributor

tgodzik commented May 19, 2023

Now this seems to work if a StoreReporter is used, but any reporter that just prints the information right away will lose the info. What is the least hacky way to get all warnings even if they were already reported?

@WojciechMazur WojciechMazur added area:semanticdb and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels May 22, 2023
@WojciechMazur
Copy link
Contributor

@tanishiking Can you look at that in your spare time? I think you had some experience with SemanticDB in the past

@tgodzik
Copy link
Contributor

tgodzik commented May 22, 2023

@tanishiking Can you look at that in your spare time? I think you had some experience with SemanticDB in the past

I think we might need to some additional output from someone closer to the core compiler. If we have a ConsoleReporter there is no way to get the warnings from pending messages, so would be great to know if we can work around it. @szymon-rd had worked recently with unused warnings, maybe he knows something?

@tanishiking
Copy link
Member

I can take a look on SemanticDB generation side, but it would be great if someone knows how can we retrieve the diagnostics via ctx

@szymon-rd
Copy link
Member

szymon-rd commented May 23, 2023

We can change how we aggregate diagnostics in the CheckUnused and linting; that change could be introduced in a patch (3.3.1?). What I find interesting is that, if I understand correctly, the semanticdb file is extracted after the typer. We don't have the lining diagnostic information at that point yet. Maybe that's even the cause of it missing?

@tgodzik
Copy link
Contributor

tgodzik commented May 23, 2023

What I find interesting is that, if I understand correctly, the semanticdb file is extracted after the typer. We don't have the lining diagnostic information at that point yet. Maybe that's even the cause of it missing?

Och, when is the linter run then? In which phase?

@szymon-rd
Copy link
Member

Most of the linting is run after the typer, but some linting requires information from inlining. Look where the CheckUnused.PostInlining is in Compiler.scala. Warnings are emitted at the end of this phase, not during the CheckUnused.PostTyper.

@tanishiking
Copy link
Member

tanishiking commented May 25, 2023

Thanks @szymon-rd !
Ok, dotty runs CheckUnused at PostTyper and PostInlining then report the intersection of both phases as unused warnings #16876. Therefore we have to extract the diagnostics into SemanticDB after the CheckUnused.PostInlining phase.

We may want to add an extra phase that extract the diagnostics and write the SemanticDB file.

bishabosha added a commit that referenced this issue Oct 19, 2023
fix #17535

## What this PR does
This PR splits `ExtractSemanticDB` to `PostTyper` and `PostInlining` (as
`ChechUnused` does).

- The `PostTyper` phase
- It extracts SemanticDB information such as symbol definitions, symbol
occurrences, type information, and synthetics.
- ~**This phase does not write the information to a `.semanticdb`
file**; instead, it attaches the SemanticDB information to the top-level
tree.~
  - And write `.semanticdb` file.
-  The `PostInlining` phase
- It extracts diagnostics from `ctx.reporter` and attaches them to the
SemanticDB information extracted in the `PostTyper` phase.
- Afterwards, it updates the SemanticDB to a `.semanticdb` file (if
there's warning in the file).
- **We need to run this phase after the `CheckUnused.PostInlining` phase
so that we can extract the warnings generated by "-Wunused".**

Also,

- Reporter now stores `warnings` in addition to `errors`
- Tweaked SemanticDBTest to show the generated diagnostics to
`metac.expect`

### Concerns

- Since we attach the SemanticDB information to the top-level tree, it
lives in-memory during the compilation which may leads to more memory
usage for compilation
- Now, Reporter stores all warnings in addition to errors (to convey the
warnings across phases), which also may cause some more memory
consumption.
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.

5 participants