Export diagnostics (including unused warnings) to SemanticDB #17835

tanishiking · 2023-06-05T07:36:35Z

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.

warnings

tanishiking · 2023-06-05T08:56:41Z

tests/semanticdb/metac.expect

@@ -3522,7 +3573,7 @@ local1 => val local right: Int
 local2 => val local number1: Int
 local3 => var local leftVar: Int
 local4 => var local rightVar: Int
-local5 => var local number1Var: Int
+local5 => val local number1Var: Int


Not sure why they became val 🤔 let me see...

It seems like -Wunused:all do the thing, maybe we can dig in another issue?

tanishiking · 2023-06-05T08:58:27Z

compiler/src/dotty/tools/dotc/semanticdb/ExtractSemanticDB.scala

+      unit.tpdTree.putAttachment(_key, extractor.toTextDocument(unit.source))
+    else
+      unit.tpdTree.getAttachment(_key) match
+        case None =>


Maybe we should say something when attachment is not found?

tanishiking · 2023-06-21T09:44:30Z

@bishabosha Could you take a look when you have time? + Is there any ways to checking the memory usage with this PR (so we can check this PR boost the memory usage of compiler) :)

bishabosha · 2023-06-21T09:50:09Z

@bishabosha Could you take a look when you have time? + Is there any ways to checking the memory usage with this PR (so we can check this PR boost the memory usage of compiler) :)

if you don't have async profiler there is also https://visualvm.github.io

tanishiking · 2023-06-22T05:30:17Z

Oh, thanks, never mind, I though by any chance, there's a benchmark or something that tracks memory consumption during the benchmark or something. I'll check using some profilers 👍

tanishiking · 2023-06-27T11:44:31Z

test performance please

tanishiking · 2023-06-27T15:32:34Z

Now, SemanticDB phases doesn't hold the semanticdb info on memory across the phases (AfterTyper - AfterInlining). Instead, ExtractSemanticDB.AfterInlining updates the SemanticDB on disk written by ExtractSemanticDB.AfterTyper, adding diagnostics.

I believe that the export diagnostics is more like IO-heavy (only if there're warnings) rather than memory-heavy.

It would be nice to test performance using test performance please, can someone run performance test?

FYI @bishabosha

bishabosha · 2023-06-28T08:46:22Z

do benchmarks exercise the extractSemanticDB phase?

Edit: short answer, no.

longer answer: looking at https://github.com/lampepfl/bench/blob/master/fixture/profiles/default.plan you can see that custom compiler flags are sent to the measure command, so anything not by default included in https://github.com/lampepfl/dotty/blob/main/bench/src/main/scala/Benchmarks.scala will not be activated.

note that zinc phases are also not turned on 🤔

tanishiking · 2023-06-29T10:04:41Z

@tgodzik This PR changes the internal interface of ExtractSemanticDB, and i realized that Metals depends on it
https://github.com/scalameta/metals/blob/6ccc512d53f231e906b9fb99a74cc774efd64fc9/mtags/src/main/scala-3/scala/meta/internal/pc/SemanticdbTextDocumentProvider.scala#L36-L37
While we can keep Extractor class public as ExtractSemanticDB.Extractor(), Metals will need to some workaround between before-this-change vs after-this-change :/

tanishiking · 2023-06-29T10:19:12Z

Thanks @bishabosha for checking!
Haven't profiled properly, but when I tested on scalameta/metals I found that it doesn't take so much (It has an overhead of 1-3 ms for compilation units with warnings, and less than 1 ms for compilation units with no warnings)

So we may not need to worry too much about performance (and I can't think of any other way to make it faster).

Now, it's ready for review 👍

tgodzik · 2023-06-29T10:44:25Z

@tgodzik This PR changes the internal interface of ExtractSemanticDB, and i realized that Metals depends on it https://github.com/scalameta/metals/blob/6ccc512d53f231e906b9fb99a74cc774efd64fc9/mtags/src/main/scala-3/scala/meta/internal/pc/SemanticdbTextDocumentProvider.scala#L36-L37 While we can keep Extractor class public as ExtractSemanticDB.Extractor(), Metals will need to some workaround between before-this-change vs after-this-change :/

We can work around that for sure, besides this will be migrating back to the compiler as soon as @rochala PR is merged.

tgodzik

Looks good from my side, reading and writing protobuf should be quick enough.

Once this PR scala/scala3#17835 has merged and released, scalafix-organize-imports should be able to run OrganizeImports.removeUnused based on the diagnostics information in SemanticDB emit from Scala3 compiler. In order to make OrganizeImports rule to work with Scala 3, this commit added a few adjustments to the rule.

bishabosha · 2023-07-14T13:38:11Z

so the ExtractSemanticDB.PostTyper phase remains the same performance as before, then the ExtractSemanticDB.PostInlining phase adds another 12% to the time taken by the prior PostTyper phase (when I ran on scala3-compiler-bootstrapped project with 579 warnings from adding -Wunused:all)

pretty much the entire cost is parsing and writing the text documents, e.g. rather than converting the warnings, so I think once warnings are converted, the reading and writing could be in parallel.

compiler/src/dotty/tools/dotc/semanticdb/ExtractSemanticDB.scala

bishabosha

implementation seems fine other than the question of performance

bishabosha

one annoying thing I found is that e.g. deprecation warnings are not included - these are emitted at crossVersionChecks which is not so long after where the phase is currently placed

compiler/src/dotty/tools/dotc/semanticdb/ExtractSemanticDB.scala

tanishiking · 2023-07-19T09:12:36Z

I think once warnings are converted, the reading and writing could be in parallel

~So, we can use runOn instead of run and process each compilation unit in parallel 👍 ~ -> d2b54c2

one annoying thing I found is that e.g. deprecation warnings are not included - these are emitted at crossVersionChecks which is not so long after where the phase is currently placed

~~Ok, I move the PostInlining phase to bit more later.~~ -> done 56c5909

Once this PR scala/scala3#17835 has merged and released, scalafix-organize-imports should be able to run OrganizeImports.removeUnused based on the diagnostics information in SemanticDB emit from Scala3 compiler. In order to make OrganizeImports rule to work with Scala 3, this commit added a few adjustments to the rule.

bjaglin · 2023-12-19T01:56:29Z

Despite being backport:nominated, it looks like this didn't make it to 3.3.2-RC1. I am not familiar with the release process, but is there a reason?

Kordyjan · 2023-12-19T10:01:55Z

Current release process is described here.
We are now entering the first iteration of the cadence. 3.3.2-RC1 is now on maven central and will be announced as stable around end of January. 3.4.0-RC1 will land tomorrow, and will be made stable around the same time as 3.3.1.

The current backlog of things to asses and backport can be found here. It is expected that there is some delay between things landing on main and shortly after in some Next release and LTS. We are moving slowly and carefully, so there are no regressions in LTS.

Some of the changes might seem obvious to just cherry-pick to the LTS branch, but there might be other changes that those changes implicitly depend on. I've seen situations like this preparing 3.3.2. This is why it is important to analyze and backport things in the same order as they were merged, even if sometimes it means that you will need to wait for needed changes slightly longer.

This particular change will land in 3.3.3 (that should have parity with 3.4.0). It will be available as an RC in late January and as a stable release in early March.

bjaglin · 2023-12-19T21:25:32Z

Many thanks for the extensive answer @Kordyjan 🙇

Once this PR scala/scala3#17835 has merged and released, scalafix-organize-imports should be able to run OrganizeImports.removeUnused based on the diagnostics information in SemanticDB emit from Scala3 compiler. In order to make OrganizeImports rule to work with Scala 3, this commit added a few adjustments to the rule.

bjaglin · 2024-02-22T20:50:31Z

This particular change will land in 3.3.3 (that should have parity with 3.4.0). It will be available as an RC in late January and as a stable release in early March.

I understand the release cycle is a bit behind, but does the target of 3.3.3 still hold despite no status update on the backport board (still in the "Needs assessment")?

Context of my question:

I am about to cut a release of Scalafix that whitelists Scala 3.4.0 thanks to this PR.
I am tempted to also whitelist the upcoming 3.3.3, in order to avoid cutting a new release a few weeks later just to allow it (since there is zero logic on the Scalafix side).
Therefore, I am trying to gauge the probability (since I understand we can't be 100% sure anyway) of the backport to happen in 3.3.3.

tgodzik · 2024-02-23T09:45:59Z

I think this should be in 3.3.3, @Kordyjan ?

scala/scala3#17835 (comment)

bjaglin · 2024-04-06T09:45:32Z

Following-up on this to make sure we don't miss the 3.3.4 opportunity (since I believe there is no technical reason that 3.3.3 didn't have it?).

I still see this PR as "Needs assessment" in the LTS backport board. I've re-read https://github.com/scala/scala3/blob/main/project/RELEASES.md#how-should-things-be-backported, but I am not sure how we can highlight a particular PR as candidate to the maintainer?

bishabosha · 2024-04-06T10:46:04Z

@Kordyjan

bjaglin · 2024-05-01T18:27:01Z

#20315 🤞

…B" (#20315) Backports #17835 to LTS

scala/scala3#17835 (comment)

tanishiking added 3 commits June 5, 2023 16:33

Export diagnostics (including unused warnings) to SemanticDB

75e6bb6

Add -Wunused:all to the compiler options to see Semanticdb export unused

c5a80c4

warnings

Add some comments

e1d0ec9

tanishiking marked this pull request as ready for review June 5, 2023 08:53

tanishiking changed the title ~~[WIP] Export diagnostics (including unused warnings) to SemanticDB~~ Export diagnostics (including unused warnings) to SemanticDB Jun 5, 2023

tanishiking commented Jun 5, 2023

View reviewed changes

jchyb requested a review from bishabosha June 5, 2023 12:11

jchyb assigned bishabosha Jun 5, 2023

ExtractSemanticDB.PostInlining now update the .semanticdb file on disk

f17fb5f

tgodzik approved these changes Jun 29, 2023

View reviewed changes

tanishiking mentioned this pull request Jun 30, 2023

Support OrganizeImports.removeUnused in Scala 3.4+ scalacenter/scalafix#1800

Merged

bishabosha reviewed Jul 14, 2023

View reviewed changes

compiler/src/dotty/tools/dotc/semanticdb/ExtractSemanticDB.scala Show resolved Hide resolved

bishabosha approved these changes Jul 14, 2023

View reviewed changes

bishabosha requested changes Jul 14, 2023

View reviewed changes

bishabosha reviewed Jul 14, 2023

View reviewed changes

compiler/src/dotty/tools/dotc/semanticdb/ExtractSemanticDB.scala Outdated Show resolved Hide resolved

bjaglin mentioned this pull request Jul 14, 2023

scala 3: RemoveUnused & OrganizeImports.removeUnused scalacenter/scalafix#1682

Closed

2 tasks

Parallelize read/write SemanticDB

d2b54c2

bjaglin mentioned this pull request Dec 11, 2023

Support RemoveUnused in Scala 3.4+ scalacenter/scalafix#1728

Merged

Kordyjan added this to the 3.4.0 milestone Dec 20, 2023

bjaglin mentioned this pull request Feb 3, 2024

Investigate false positive unused import in Scala 3 bjaglin/scalafix#5

Closed

bjaglin mentioned this pull request Feb 26, 2024

Add 3.4.0 blogpost scala/scala-lang#1598

Merged

bjaglin added a commit to bjaglin/scalafix that referenced this pull request Feb 26, 2024

support upcoming 3.3.3 for unused warnings

82b677d

scala/scala3#17835 (comment)

bjaglin mentioned this pull request Feb 26, 2024

support upcoming 3.3.3 for unused warnings scalacenter/scalafix#1941

Closed

bjaglin added a commit to scalacenter/scalafix that referenced this pull request Feb 26, 2024

support upcoming 3.3.3 for unused warnings

21a5bdd

scala/scala3#17835 (comment)

bjaglin added a commit to bjaglin/scalafix that referenced this pull request Feb 28, 2024

support upcoming 3.3.4 for unused warnings

b582487

scala/scala3#17835 (comment)

This was referenced Feb 28, 2024

handle unused warnings for sources compiled with upcoming LTS 3.3.4 scalacenter/scalafix#1942

Merged

Scala3: test against both LTS & Next, build with LTS scalacenter/scalafix#1926

Merged

bjaglin mentioned this pull request May 1, 2024

Backport "Export diagnostics (including unused warnings) to SemanticDB" #20315

Merged

WojciechMazur added a commit that referenced this pull request Jun 22, 2024

Backport "Export diagnostics (including unused warnings) to SemanticD…

b26ae8e

…B" (#20315) Backports #17835 to LTS

bjaglin added a commit to scalacenter/scalafix that referenced this pull request Jun 23, 2024

support upcoming 3.3.4 for unused warnings

21db4ae

scala/scala3#17835 (comment)

bjaglin added a commit to scalacenter/scalafix that referenced this pull request Jun 26, 2024

support upcoming 3.3.4 for unused warnings

1ba96c3

scala/scala3#17835 (comment)

bjaglin added a commit to bjaglin/scalafix that referenced this pull request Jul 29, 2024

support upcoming 3.3.4 for unused warnings

6aed93b

scala/scala3#17835 (comment)

bjaglin added a commit to bjaglin/scalafix that referenced this pull request Jul 29, 2024

support upcoming 3.3.4 for unused warnings

7a9d9e4

scala/scala3#17835 (comment)

bjaglin added a commit to scalacenter/scalafix that referenced this pull request Aug 1, 2024

handle unused warnings for sources compiled with upcoming LTS 3.3.4

28e9fcd

scala/scala3#17835 (comment)

WojciechMazur removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export diagnostics (including unused warnings) to SemanticDB #17835

Export diagnostics (including unused warnings) to SemanticDB #17835

tanishiking commented Jun 5, 2023 •

edited

Loading

tanishiking Jun 5, 2023

tanishiking Jun 27, 2023

tanishiking Jun 5, 2023

tanishiking commented Jun 21, 2023

bishabosha commented Jun 21, 2023

tanishiking commented Jun 22, 2023

tanishiking commented Jun 27, 2023

tanishiking commented Jun 27, 2023 •

edited

Loading

bishabosha commented Jun 28, 2023 •

edited

Loading

tanishiking commented Jun 29, 2023

tanishiking commented Jun 29, 2023

tgodzik commented Jun 29, 2023

tgodzik left a comment

bishabosha commented Jul 14, 2023 •

edited

Loading

bishabosha left a comment

bishabosha left a comment •

edited

Loading

tanishiking commented Jul 19, 2023 •

edited

Loading

bjaglin commented Dec 19, 2023

Kordyjan commented Dec 19, 2023

bjaglin commented Dec 19, 2023

bjaglin commented Feb 22, 2024

tgodzik commented Feb 23, 2024

bjaglin commented Apr 6, 2024

bishabosha commented Apr 6, 2024

bjaglin commented May 1, 2024 •

edited

Loading

Export diagnostics (including unused warnings) to SemanticDB #17835

Export diagnostics (including unused warnings) to SemanticDB #17835

Conversation

tanishiking commented Jun 5, 2023 • edited Loading

What this PR does

Concerns

tanishiking Jun 5, 2023

Choose a reason for hiding this comment

tanishiking Jun 27, 2023

Choose a reason for hiding this comment

tanishiking Jun 5, 2023

Choose a reason for hiding this comment

tanishiking commented Jun 21, 2023

bishabosha commented Jun 21, 2023

tanishiking commented Jun 22, 2023

tanishiking commented Jun 27, 2023

tanishiking commented Jun 27, 2023 • edited Loading

bishabosha commented Jun 28, 2023 • edited Loading

tanishiking commented Jun 29, 2023

tanishiking commented Jun 29, 2023

tgodzik commented Jun 29, 2023

tgodzik left a comment

Choose a reason for hiding this comment

bishabosha commented Jul 14, 2023 • edited Loading

bishabosha left a comment

Choose a reason for hiding this comment

bishabosha left a comment • edited Loading

Choose a reason for hiding this comment

tanishiking commented Jul 19, 2023 • edited Loading

bjaglin commented Dec 19, 2023

Kordyjan commented Dec 19, 2023

bjaglin commented Dec 19, 2023

bjaglin commented Feb 22, 2024

tgodzik commented Feb 23, 2024

bjaglin commented Apr 6, 2024

bishabosha commented Apr 6, 2024

bjaglin commented May 1, 2024 • edited Loading

tanishiking commented Jun 5, 2023 •

edited

Loading

tanishiking commented Jun 27, 2023 •

edited

Loading

bishabosha commented Jun 28, 2023 •

edited

Loading

bishabosha commented Jul 14, 2023 •

edited

Loading

bishabosha left a comment •

edited

Loading

tanishiking commented Jul 19, 2023 •

edited

Loading

bjaglin commented May 1, 2024 •

edited

Loading