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.

Sign up for GitHub

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

Jump to bottom

record end markers in trees and semanticdb, exclude top level def wrappers #12541

Merged
merged 8 commits into from
Jun 1, 2021
Merged

record end markers in trees and semanticdb, exclude top level def wrappers #12541

merged 8 commits into from
Jun 1, 2021

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented May 20, 2021 •
edited
Loading

fixes #12326
fixes #11693

modify spans of all trees with end markers.
record an end marker was present for trees that reference symbols

also prevent top level definition wrapper objects appearing in occurrences (as a definition, but record explicit references)

@bishabosha bishabosha requested review from smarter and tgodzik May 20, 2021 13:56
tgodzik
tgodzik approved these changes May 20, 2021
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

@bishabosha bishabosha marked this pull request as draft May 20, 2021 16:27
@bishabosha
Copy link
Member Author

bishabosha commented May 20, 2021 •
edited
Loading

looks like it doesnt work for object defs

Edit now it works

@bishabosha bishabosha marked this pull request as ready for review May 20, 2021 17:54
@bishabosha
Copy link
Member Author

also added fixes for nameSpan of package objects and sorted endSpan for module defs

smarter
smarter requested changes May 26, 2021
View reviewed changes
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

I think we should take the time to discuss the overall approach here at the next Monday meeting. I think it's fine but it's worth asking whether this is ideal.

compiler/src/dotty/tools/dotc/parsing/Parsers.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/parsing/Parsers.scala Outdated Show resolved Hide resolved
@bishabosha bishabosha requested a review from smarter May 31, 2021 15:43
smarter
smarter requested changes May 31, 2021
View reviewed changes
compiler/src/dotty/tools/dotc/ast/Desugar.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/ast/Trees.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/ast/Trees.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/ast/Trees.scala Outdated Show resolved Hide resolved
@smarter smarter assigned bishabosha and unassigned smarter May 31, 2021
@bishabosha
Copy link
Member Author

I have thought that this isn't going to work when compiling from tasty - although we have never tested semanticdb idempotency from tasty anyway

@smarter
Copy link
Member

smarter commented May 31, 2021

True, we'd need to store the EndIndex in tasty too to support that (by the way, do we actually need the offset? Seems like we could get by with only one boolean to know whether there is an end marker or not, then just assume the offset corresponds to the tree span end offset).

@bishabosha
Copy link
Member Author

bishabosha commented Jun 1, 2021 •
edited
Loading

(by the way, do we actually need the offset? Seems like we could get by with only one boolean to know whether there is an end marker or not, then just assume the offset corresponds to the tree span end offset).

This does not work presently, unless we modify the span of the tree after checking that the end marker is correctly positioned (otherwise the end of the tree span is before the end marker)

@smarter
Copy link
Member

smarter commented Jun 1, 2021

otherwise the end of the tree span is before the end marker

Ah, I didn't know that, I would expect it to be part of the span indeed, that could be useful for other IDE operations like commenting out a whole definition

@bishabosha bishabosha requested a review from smarter June 1, 2021 12:34
@bishabosha bishabosha force-pushed the fix-12326 branch 2 times, most recently from d839dde to 049904a Compare June 1, 2021 12:40
@bishabosha
Copy link
Member Author

I have changed so that now all statements will receive a modified span, and WithEndMarker now just controls addition of a single unit attachment.

smarter
smarter approved these changes Jun 1, 2021
View reviewed changes
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

LGTM, but we should still strive to have all IDE-relevant information in Tasty, so maybe open an issue about how to preserve WithEndMarker? Maybe we could use a flag for that.

@bishabosha bishabosha requested review from tgodzik and smarter June 1, 2021 13:56
@bishabosha bishabosha assigned smarter and unassigned bishabosha Jun 1, 2021
@bishabosha
Copy link
Member Author

sorry I snuck in another last minute change to deal with #11693 @tgodzik

@bishabosha bishabosha changed the title record end markers in trees and semanticdb record end markers in trees and semanticdb, exclude top level def wrapper objects Jun 1, 2021
@bishabosha bishabosha changed the title record end markers in trees and semanticdb, exclude top level def wrapper objects record end markers in trees and semanticdb, exclude top level def wrappers Jun 1, 2021
@smarter
Copy link
Member

smarter commented Jun 1, 2021 •
edited
Loading

I get that recording this occurrence isn't particularly useful, but its it incorrect to have it in semanticdb? The occurence is real, it's just not visible at source-level, but the same is true for many other things generated by the compiler.

tgodzik
tgodzik approved these changes Jun 1, 2021
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@bishabosha
Copy link
Member Author

I get that recording this occurrence isn't particularly useful, but its it incorrect to have it in semanticdb? The occurence is real, it's just not visible at source-level, but the same is true for many other things generated by the compiler.

As far as I know - occurrences are only for visible references in the source code - many things are excluded because of this - we still record that the wrapper exists in the Symbols section

@tgodzik
Copy link
Contributor

tgodzik commented Jun 1, 2021

I get that recording this occurrence isn't particularly useful, but its it incorrect to have it in semanticdb? The occurence is real, it's just not visible at source-level, but the same is true for many other things generated by the compiler.

it's just not visible at source-level, <- the intent of semanticdb occurrences is only to record things that are visible at source-level, which is most useful for us. Anything that is not visible should go to synthetics, so things that are added by the compiler.

Edit: Or symbols as Jamie explained.

@bishabosha
Copy link
Member Author

bishabosha commented Jun 1, 2021 •
edited
Loading

Anything that is not visible should go to synthetics, so things that are added by the compiler.

perhaps we kept it in the past because there was (still is at this time) no synthetics section?

@tgodzik
Copy link
Contributor

tgodzik commented Jun 1, 2021

Anything that is not visible should go to synthetics, so things that are added by the compiler.

perhaps we kept it in the past because there was (still is at this time) no synthetics section?

Most likely, but they were not used by Metals or any other tool using semanticdb.

@bishabosha bishabosha mentioned this pull request Jun 1, 2021
Closed
@bishabosha
exclude top level wrapper objects
c327d79 
set the Synthetic flag on
top level wrapper objects.

fixes scala#11693, top level definition
wrapper objects no longer appear
as a definition occurrence in semanticdb,
but are still recorded as a symbol
@bishabosha bishabosha requested a review from smarter June 1, 2021 14:31
@bishabosha bishabosha assigned bishabosha and unassigned smarter Jun 1, 2021
@bishabosha bishabosha enabled auto-merge June 1, 2021 14:38
@bishabosha bishabosha merged commit 85575cc into scala:master Jun 1, 2021
@bishabosha bishabosha deleted the fix-12326 branch June 1, 2021 15:40
@scala scala deleted a comment from tgodzik Jun 2, 2021
@griggt griggt mentioned this pull request Jul 6, 2021
Closed
@Kordyjan Kordyjan added this to the 3.0.2 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgodzik tgodzik tgodzik approved these changes

@smarter smarter smarter approved these changes

Assignees

@bishabosha bishabosha

Labels
area:semanticdb
Projects
None yet
Milestone
3.0.2
Development

Successfully merging this pull request may close these issues.

Include end marker name in semanticdb [Semanticdb] Package object occurrence shown for toplevel given
4 participants
@bishabosha @smarter @tgodzik @Kordyjan