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

Add access information to SymbolInformation #12964

Merged
merged 3 commits into from
Jul 1, 2021

Conversation

tanishiking
Copy link
Member

#12963

This PR adds access information to SymbolInformation. I confirm the diff is legit just by thoroughly check the diff in metac.expect.

(but, it might be better to have a script to verify the Scala2 and Scala3 will extract the same SemanticDB for the same program, so we can use the scala2's output as a test oracle 🤔 )(as long as the program compiles both in Scala2 and Scala3)

Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

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

Thank you for taking this on!
There seems to be an issue with private-qualified/protected-qualified that does not match my expectation

case _: PrivateThisAccess => "private[this] "
case _: ProtectedThisAccess => "protected[this] "
case PrivateWithinAccess(ssym) =>
s"private[${ssym}] "
Copy link
Member

Choose a reason for hiding this comment

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

worth noting here that the official metap will print the displayName of the associated SymbolInformation, and we have been trying to replicate its output style as much as possible

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the situation will be better with #12885 but I was just a bit lazy to duplicate the pretty printer to this PR... Can I go with this and prettify it in the future?

Copy link
Member

Choose a reason for hiding this comment

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

yep, this is fine for now

@@ -119,7 +133,7 @@ object Tools:
case TRAIT => sb.append("trait ")
case INTERFACE => sb.append("interface ")
case UNKNOWN_KIND | Unrecognized(_) => sb.append("unknown ")
sb.append(info.displayName).nl
sb.append(s"${accessString(info.access)}${info.displayName}").nl
Copy link
Member

Choose a reason for hiding this comment

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

Here I think printing info.access should come before printing info.kind (private[example] val method foo rather than val method private[example] foo)

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 338e6d2

example/Access#m6(). => method m6
example/Access#m1(). => method private[this] m1
example/Access#m2(). => method private[this] m2
example/Access#m3(). => method protected[example/Access#] m3
Copy link
Member

Choose a reason for hiding this comment

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

if we look in tests/semanticdb/expect/Access.scala this should be private qualified not protected qualified

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed by #12964 (comment)

else PublicAccess()
else
val ssym = symbolName(sym.privateWithin)
if (sym.isPrivate) PrivateWithinAccess(ssym)
Copy link
Member

Choose a reason for hiding this comment

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

I think here that a private qualified symbol never gets the private flag, you should instead check if it is protected. See the other comment about the metac.expect output for tests/semanticdb/expect/Access.scala

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're right, fixed in fb6b679 👍

flags/p/package.`y_=`().(x$1) => param x$1
flags/p/package.m(). => macro m
flags/p/package.m().[TT] => typeparam TT
flags/p/package.x. => lazy val method x
flags/p/package.x. => lazy val method protected[flags/p/] x
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is right, in scala 2 semanticdb the access is printed as private, and the definition in tests/semanticdb/expect/semanticdb-Flags.scala is private lazy val x = 1

Copy link
Member

@bishabosha bishabosha Jun 28, 2021

Choose a reason for hiding this comment

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

to debug you can try using this script I made.

If I run it with

scala3-compiler-bootstrapped/Test/runMain dotty.tools.printSymFlags "package flags { package object p { private lazy val x = 1 }}" "x"

I get the output

val x => lazy <touched> private[module class p]
// compiler/test/dotty/tools/DottySymbolStealer.scala
package dotty.tools

import dotc.ast.tpd
import dotc.core.Names._
import dotc.ast.tpd._
import dotc.core.Contexts.Context
import dotc.core.Symbols.Symbol
import dotc.core.Decorators._
import dotc.core.Types.Type

@main def printSymFlags(source: String, termRefs: String*) = {
  val (ctx, syms) = DottySymbolStealer.stealSymbol(source, termRefs*)
  given Context = ctx
  syms.map(sym =>
    val priv = sym.privateWithin
    s"$sym => ${sym.flags.flagStrings(if priv.exists then s"$priv" else "").mkString(" ")}"
  ).foreach(println)
}

object DottySymbolStealer extends DottyTest {
  def stealSymbol(source: String, termRefs: String*): (Context, List[Symbol]) = {
    var scontext : Context = null
    var tp: List[Symbol] = null
    checkCompile("typer", source) {
      (tree, context) =>
        given Context = context
        val remaining = scala.collection.mutable.Set.from(termRefs)
        val findDef: (List[ValOrDefDef], tpd.Tree) => List[ValOrDefDef] =
          (acc , tree) =>  tree match {
            case t: ValOrDefDef =>
              remaining.find(t.name.toString == _) match
                case Some(name) =>
                  remaining -= name
                  t :: acc
                case _ =>
                  acc
            case _ => acc
          }
        val d = new DeepFolder[List[ValOrDefDef]](findDef).foldOver(Nil, tree)
        tp = d.map(_.symbol).reverse
        scontext = context
    }
    (scontext, tp)
  }
}

Copy link
Member

@bishabosha bishabosha Jun 28, 2021

Choose a reason for hiding this comment

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

so using scala 2 repl with the power mode it appears that private values in a package object do not get a privateWithin symbol, but I think its ok to keep it for scala 3, however it still needs to be private[p] not protected[p]

For `protected[within]`, the symbol has `protected` flag,
but `private[within]` doesn't have flag related to access information.
Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

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

This looks great to me, thank you

@bishabosha bishabosha merged commit 9f97b0b into scala:master Jul 1, 2021
@tanishiking tanishiking deleted the access-info branch August 5, 2021 04:32
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants