Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Export diagnostics (including unused warnings) to SemanticDB #17835
Export diagnostics (including unused warnings) to SemanticDB #17835
Changes from 7 commits
75e6bb6
c5a80c4
e1d0ec9
f17fb5f
d2b54c2
56c5909
50e0fd2
2babbc4
5186b62
66a5306
a414fae
d5065ec
d7258b4
4b5d3e7
3008ed8
c9de8e2
40a2a00
7b29f4c
4f6a092
a6dfec2
3daeaa6
499c347
71a5cd0
b28b425
d54d8a2
b8565a0
2857632
053e644
567d486
275e6fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are java protobuf classes - you should be able to get a slightly better syntax (but no significant difference in performance) with classes in the
scalapb
packageThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we cannot utilize the
scalapb
runtime in this context. We intentionally removed the dependency on the scalapb-runtime package insemanticdb-for-scala3.
The reason behind this decision is to avoid including the
scalapb
runtime and its transitive dependencies from the Scala 3 compiler.(TBH, I'm not sure why the compiler has a dependency on
com.google.protobuf
🤔 )There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesnt work anyway,
If we want to do this then we should copy these definitions to the
dotty.tools.dotc.semanticdb.internal
packageThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there used to be a dependency on protobuf due to Zinc 1.3, but we merged yesterday depending exclusively on Zinc 1.9 which no longer has protobuf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
com.google.protobuf
removed recently? It works in my local environment 🤔Ah, ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure wether should we do without
com.google.protobuf
.Should we still being performant by reading protobuf without parsing? (I know it's better, but should we do that later in another PR?)
I gonna look into is there an API that reads protobuf as an Empty in
scalapb
that doesn't depend oncom.google.protobuf
, but it may take some time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we merge the current state or one with the fixes? This would unblock scalafix, so would be pretty great to do. We could do optimization improvements later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move back to "parsing TextDocuments and update it" anyway :) -> done d5065ec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also I think here we should extract the unit source, and do the conversion to SemanticDiagnostic before we start parallelising
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not quite sure but there could be a slight difference here given that the extractor isn't running with
unitCtx
like beforeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so to make this safe to call in parallel we really need to get rid of that
Context
parameter, and it looks like its only required forsemanticdbPath
, so perhaps we can compute that before calling the methodThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done: d7258b4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am AFK so I can't confirm, but I believe you should pass to
addLengthDelimited
the (ByteString
) concatenation ofdocBytes.get(0)
anddoc.toByteString
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should start from a fresh
TextDocuments
instead of reusing the existing one - the current state will be carried over in the unknown fieldThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this does not need context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed for
relPath
and
semanticdbTarget
Should we pass around
settings
instead ofusing Context
?