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

Use slash / for package symbols #1660

Merged
merged 12 commits into from
Jul 3, 2018
Merged

Use slash / for package symbols #1660

merged 12 commits into from
Jul 3, 2018

Conversation

olafurpg
Copy link
Member

@olafurpg olafurpg commented Jul 3, 2018 •
edited
Loading

Fixes #1647.
Fixes #1655 (remove redundant unit tests)
Fixes #1420 (package symbol language)

Currently, SemanticDB package symbols use . like for scala in scala.Predef.String#. This is problematic since it's not possible to distinguish packages from Scala objects like Predef. This PR follows up on the proposal in #1647 to change the format to be scala/Predef.String# instead. The benefit of this strategy is that we no longer need to persist package symbols and we can compute their SymbolInformation with a stateless method like this

def info(symbol: String): SymbolInformation =
  if (symbol.endsWith("/")) SymbolInformation(symbol, k.PACKAGE)
  else ...

@olafurpg olafurpg requested a review from xeno-by July 3, 2018 14:20
@olafurpg olafurpg mentioned this pull request Jul 3, 2018
Closed
olafurpg
olafurpg commented Jul 3, 2018
View reviewed changes
tests/jvm/src/test/resources/metac-metacp.diff Outdated
+++ metacp
singleType {
prefix {
- symbol: "scala/util/Either."
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only regression I can see, and the issue is already tracked in #1625 (comment) I will fix that issue later this week.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out #1625 was unrelated after all, this regression has been fixed now.

@olafurpg
Copy link
Member Author

olafurpg commented Jul 3, 2018

Seems like my fix for scala/util.Either. by removing hasTermName introduced a regression

scala.meta.internal.scalacp.MissingSymbolException: scala.reflect.macros.blackbox

I will investigate later.

@olafurpg
Use slash / for package symbols
0f4b679
@olafurpg
Update expect tests
2094ebe
@olafurpg
Remove redundant unit tests, fixes scalameta#1655
0e5cac4 
Converts the interesting unit tests into expect tests.
@olafurpg
Update spec to use slashes for package symbols
c255d11
@olafurpg
Fix leftover java.lang.Class
430c13d
@olafurpg
Reset package names for targeted tests.
5b4c02c
@olafurpg
Fix broken CliSuite
94881d3
@olafurpg
Remove unnecessary hasTermName trick in metacp
1e3a73b 
Now that we have the capability to query the classpath if a symbol is a
package we no longer need to resort to "smart tricks".
@olafurpg
Revert previous commit.
0c5c84b 
hasTermName seems to be required to catch a couple object symbols.
olafurpg
olafurpg commented Jul 3, 2018
View reviewed changes
semanticdb/metacp/src/main/scala/scala/meta/internal/scalacp/SymbolInformationOps.scala
@@ -67,7 +67,8 @@ trait SymbolInformationOps { self: Scalacp =>
}
val isModuleClass = sym.entry.entryType == 10
def isScalaObject: Boolean = isModuleClass && !sym.isJavaDefined
if (hasTermName || isScalaObject) k.OBJECT
if (sym.isPackageAccordingToClasspath) k.PACKAGE
else if (hasTermName || isScalaObject) k.OBJECT
Copy link
Member Author

Choose a reason for hiding this comment

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

You can ignore the commit 0c5c84b. I got greedy in 1e3a73b and tried to fix an issue unrelated to this PR.

@olafurpg olafurpg mentioned this pull request Jul 3, 2018
Closed
xeno-by
xeno-by requested changes Jul 3, 2018
View reviewed changes
semanticdb/semanticdb3/semanticdb3.md Outdated
@@ -2377,8 +2377,8 @@ In this section, we describe the Java symbol format.

**Descriptor** is:
* For `LOCAL`, unsupported.
* For `FIELD` or `PACKAGE`, concatenation of its simple name [\[98\]][98]
and a dot (`.`).
* For `PACKAGE`, concatenation of its encoded name and a forward slash (`/`).
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like the Java part of the spec defines what's an encoded name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

tests/jvm/src/test/resources/metac-metacp.diff
@@ -291,13 +291,13 @@ com.javacp.ClassSuffix#Inner#`<init>`().
- tag: PUBLIC
- symbol: ""
+ tag: PRIVATE_WITHIN
+ symbol: "com.javacp."
+ symbol: "com/javacp/"
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

The change is expected, it used to be com.javacp. and is now com/javacp/. Observe that the diff is only one line, the PRIVATE_WITHIN diff is from the diff file itself, it's unchanged in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right.

@xeno-by xeno-by added this to the 4.0.0-RC1 milestone Jul 3, 2018
@xeno-by xeno-by self-assigned this Jul 3, 2018
olafurpg
olafurpg commented Jul 3, 2018
View reviewed changes
Copy link
Member Author

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

I've updated the Java spec to use "encoded name" everywhere.

@olafurpg olafurpg merged commit 9dffa84 into scalameta:master Jul 3, 2018
@olafurpg olafurpg deleted the package-slash branch July 3, 2018 22:06
@olafurpg olafurpg mentioned this pull request Jul 3, 2018
Merged
xeno-by added a commit that referenced this pull request Jul 4, 2018
@xeno-by
Merge pull request #1670 from olafurpg/slash-followup
155edb2 
Several followup fixes from #1660
@olafurpg olafurpg mentioned this pull request Jul 5, 2018
Closed
@xeno-by xeno-by mentioned this pull request Jul 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xeno-by xeno-by xeno-by approved these changes

Assignees

@xeno-by xeno-by

Labels
None yet
Projects
None yet
Milestone
4.0.0-RC1
Development

Successfully merging this pull request may close these issues.
2 participants
@olafurpg @xeno-by