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

Support reporting of all compilation problems #2297

Merged
merged 7 commits into from
Feb 10, 2023

Conversation

lefou
Copy link
Member

@lefou lefou commented Jan 31, 2023

In it's incremental nature, Zinc does only report problems for re-compiled files,
which leads to situations, where you only see a glimpse of the overall compiler warnings.

Now, we use the Zinc internal analysis to recover all found problems. We also record all already shown problems and simply log the not shown but still valid problems after the compilation finished.

This behavior can be configured by the new target JavaModule.zincReportCachedProblems: T[Boolean]. It defaults to false to keep backward-compatible output.

In it's incremental nature, Zinc does only report problems for re-compiled files,
which leads to situations, where you only see a glimpse of the overall compiler warnings.

With this change, I keep a record of all reported problems, so they can be later re-shown to the user.
The cache is dropping all recorded problems for files, which were recompiled.
@lefou
Copy link
Member Author

lefou commented Feb 1, 2023

The output in the first draft isn't correct yet. Also, I think we probably can also access complete problems via the internal compiler analysis, which need to be handled in the worker classpath though. I think I will change the design again.

@lefou
Copy link
Member Author

lefou commented Feb 1, 2023

Yeah, ZincWorkerImpl.discoverMainClasses also uses the zinc analysis, and the SourceInfo interface seems to contain problems for all source files, including those not recompiled.

lefou added 2 commits February 1, 2023 23:35
We now use the Zinc compile analysis to recover older problems.

Added a new `zincReportOldProblems` target to `JavaModule`.
@lefou lefou marked this pull request as ready for review February 1, 2023 23:00
@lefou lefou changed the title Rudimentary cache for zinc compilation problems Support reporting of all compilation problems Feb 2, 2023
@lefou lefou requested a review from lolgab February 2, 2023 08:50
@lefou
Copy link
Member Author

lefou commented Feb 2, 2023

I changed the design and it seems to work reasonable now. Not sure about the name and the default value of configuration target zincReportOldProblems.

@@ -198,7 +198,8 @@ trait SemanticDbJavaModule extends CoursierModule { hostModule: JavaModule =>
scalacOptions = scalacOptions,
compilerClasspath = m.scalaCompilerClasspath(),
scalacPluginClasspath = semanticDbPluginClasspath(),
reporter = T.reporter.apply(hashCode)
reporter = T.reporter.apply(hashCode),
reportOldProblems = false
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial though was, that semanticdb data is used via Metals, and I think BSP clients are persisting problems anyway. Problems are send as events and not as a result of a compilation, so the client need to manage them anyway.

But OTOH, when semanticdb data is needed for other use, e.g. scalafix, warnings might be still of interest. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if Metals can correctly handle duplicated problems.
Maybe @tgodzik can answer this?
About Scalafix, having the warnings reported again would be good.
But this decision can also be taken in a second moment. We can merge this as false and then experiment in a second PR to see the implications for BSP.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will show all the diagnostics that we are sent until we get reset in PublishDiagnosticsParams , since we don't know if problems were solved until we are told so explicitely.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgodzik Thanks Tomasz! That means, in case of incremental compilation, diagnostics where until now either incomplete (if there was a reset) or may even contain outdated or duplicate problems (if there was no reset). I think, always respecting the setting of zincReportOldProblems is probably the cleanest option.

@lefou
Copy link
Member Author

lefou commented Feb 10, 2023

This PR is now complete. I updated the description.

@lefou lefou merged commit cbb222b into com-lihaoyi:main Feb 10, 2023
@lefou lefou deleted the cache-zinc-problems branch February 10, 2023 08:20
@lefou lefou added this to the 0.11.0-M4 milestone Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants