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

SI-9437 Emit and support parameter names in class files #4735

Merged
merged 1 commit into from Jan 26, 2016

Conversation

Projects
None yet
6 participants
@soc
Copy link
Member

soc commented Sep 9, 2015

JEP 118 added a MethodParameters attribute to the class file spec which
holds the parameter names of methods when compiling Java code with
javac -parameters.

@scala-jenkins scala-jenkins added this to the 2.12.0-M3 milestone Sep 9, 2015

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Sep 9, 2015

This is still a bit WIP as tests need to be written.

I added the change with an explicit setting, but @Ichoran (and me) think that it would make sense to just start supporting it by default, and not introducing an option, given that scalac seemed to accept named parameters for methods originating from Java source files since the beginning anyway.

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Sep 9, 2015

I'm storing the decoded names in the attribute (which means the unmangled names) for slightly nicer Java interop, not sure if this will cause issues in other parts ...

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Sep 9, 2015

See also: https://bugs.openjdk.java.net/secure/attachment/11079/8misc.pdf for more nitty gritty.

Looks like the Java standard library isn't compiled with -parameters, so we still won't be able to call it with named params:

scala> classOf[java.util.List[_]].getDeclaredMethods.head
res0: java.lang.reflect.Method = public abstract boolean java.util.List.add(java.lang.Object)

scala> .getParameters.head
res1: java.lang.reflect.Parameter = E arg0

scala> .isNamePresent
res2: Boolean = false
@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Sep 21, 2015

@soc looks great! It seems that the Java source parser already keeps the names, so nothing to fix there. I agree that we can skip the flag: named arguments already work today when parsing Java from source.

@lrytz lrytz modified the milestones: 2.12.0-M3, 2.12.0-M4 Oct 5, 2015

@SethTisue SethTisue added the on-hold label Oct 27, 2015

@soc soc force-pushed the soc:SI-9437 branch from 05daf47 to 2765a96 Oct 31, 2015

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Oct 31, 2015

Updated the PR to remove the setting, and added a test.

Review by @lrytz?

@soc soc force-pushed the soc:SI-9437 branch from 2765a96 to 1646a74 Oct 31, 2015

@SethTisue SethTisue removed the on-hold label Nov 1, 2015

@soc soc force-pushed the soc:SI-9437 branch from 1646a74 to a0a4e70 Nov 16, 2015

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Nov 16, 2015

@lrytz Updated.

for (param <- params) {
var access = asm.Opcodes.ACC_FINAL
if (param.mods.isSynthetic)
access |= asm.Opcodes.ACC_SYNTHETIC

This comment has been minimized.

@lrytz

lrytz Nov 16, 2015

Member

echoing my earlier comment:

This is a known area of confusion in scalac: we have two flags for symbols SYNTHETIC and ARTIFACT. The distinction is that only things marked ARTIFACT should be marked ACC_SYNTHETIC in the bytecode, see https://github.com/scala/scala/blob/2.12.x/src/reflect/scala/reflect/api/FlagSets.scala#L229-L248 and https://github.com/scala/scala/blob/2.12.x/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala#L310.

It is neither well-specified nor well-tested what definitions in a classfile exactly get the ACC_SYNTHETIC, but in order to retain at least the current consistency we should only add the flag here for params that are isArtifact.

Also, another non-obvious thing: instead of looking at the flags in mods, you should look at the symbol (param.symbol.isArtifact); the modifiers are generally used until Typer, after that the symbol should be the main source of information.

cc @retronym – please correct me if I'm wrong!

Thinking a bit more, using ARTIFACT would mean that a lot of synthetic params are not marked ACC_SYNTHETIC: newSyntheticValueParams marks the symbols SYNTHETIC only, it's used for example for default getters and delambdafy methods.

Given that we already don't emit ACC_SYNTHETIC for a lot of scala-synthetic definitions (default getter methods for example), I'd say that's fine, we're not making the situation fundamentally worse.

This comment has been minimized.

@lrytz

lrytz Dec 18, 2015

Member

To summarize: my suggestion here is to change the above condition to if (param.symbol.isArtifact). The consequence would be that params synthesized by newSyntheticValueParams would not be marked synthetic in the bytecode, which is in line with what we are doing in many other cases (e.g. delambdafy methods).

@soc soc force-pushed the soc:SI-9437 branch from a0a4e70 to b0595a0 Dec 18, 2015

@soc soc changed the title SI-9437 Add support for parameter names in class files SI-9437 Emit and support parameter names in class files Dec 18, 2015

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Dec 18, 2015

@lrytz Updated! I added tests, made the isArtifact change.

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Dec 31, 2015

Argh, WTH just happened?

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Jan 4, 2016

What's the reason for having two tests (t9437a / t9437b)?

I was also wondering if there could be a test for calling a Java-defined method from Scala with named arguments - I don't know of any way to make current partest pass -parameters to javac.

@soc soc force-pushed the soc:SI-9437 branch from c699adc to 1c4c2f3 Jan 5, 2016

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 5, 2016

It tests parameter names coming from different places because they can enter the compiler via different frontends, either directly via Scala source code, Scala Signatures, Java class files (not tested).

Without a supply of Java class files with parameter names, it's pointless to call them with named arguments, the information is not there and scalac will just invent names like x$1 on the fly.

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Jan 5, 2016

Not sure if this is helpful here, but in a previous change to support Java 8 bytecode before we mandated running on Java 8, we used ASM to create classfiles programatically with default methods and fed these into the classpath of programatically driven scalac: fc6da8d

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Jan 5, 2016

It tests parameter names coming from different places because they can enter the compiler via different frontends, either directly via Scala source code, Scala Signatures, Java class files (not tested).

I don't see how this is the case, both tests use java reflection to get the parameter names from the classfile, no?

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Jan 6, 2016

More concretely, I was looking for a way to address:

Without a supply of Java class files with parameter names, it's pointless to call them with named arguments

by creating a Java class file with parameter names, and ensuring that we can use Scala's named arguments to call it.

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 7, 2016

@lrytz You are correct. The tests are missing a line were they actually call those methods ...

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 7, 2016

@lrytz @retronym Thanks for pushing this forward! The Java test uncovered a small bug, so it already paid itself! :-) The changes are in the new commit. If this looks fine, I'll squash it!

@soc soc force-pushed the soc:SI-9437 branch from 26cf4ea to 3ed1075 Jan 7, 2016

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 9, 2016

@lrytz Does this look fine?

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Jan 11, 2016

@soc yes it looks great! you can squash the two commits.

@soc soc force-pushed the soc:SI-9437 branch from 3ed1075 to ad3d599 Jan 11, 2016

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 11, 2016

@lrytz Done!

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Jan 12, 2016

LGTM, great work!

@soc soc force-pushed the soc:SI-9437 branch from ad3d599 to 260df93 Jan 14, 2016

for (param <- params) {
var access = asm.Opcodes.ACC_FINAL
if (param.mods.isArtifact)
access |= asm.Opcodes.ACC_SYNTHETIC

This comment has been minimized.

@retronym

retronym Jan 19, 2016

Member

It seems odd to me that you're looking up flags and the name from ValDef#{name, mods}, rather than from getting then from the Symbol. Usually, after typechecking, the Symbol is the used as the source of truth.

@soc soc force-pushed the soc:SI-9437 branch from 260df93 to ecc0ef9 Jan 23, 2016

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 23, 2016

@retronym Is this fine? Will squash afterwards.

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Jan 25, 2016

thanks @soc, it looks good - please squash the two commits

SI-9437 Emit and support parameter names in class files
JEP 118 added a MethodParameters attribute to the class file spec which
holds the parameter names of methods when compiling Java code with
`javac -parameters`.

We emit parameter names by default now.

@soc soc force-pushed the soc:SI-9437 branch from ecc0ef9 to c78d771 Jan 25, 2016

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 25, 2016

@lrytz Thanks, done.

lrytz added a commit that referenced this pull request Jan 26, 2016

Merge pull request #4735 from soc/SI-9437
SI-9437 Emit and support parameter names in class files

@lrytz lrytz merged commit bbd890b into scala:2.12.x Jan 26, 2016

5 checks passed

cla @soc signed the Scala CLA. Thanks!
Details
integrate-ide [856] SUCCESS. Took 6 s.
Details
validate-main [1050] SUCCESS. Took 199 min.
Details
validate-publish-core [1036] SUCCESS. Took 29 min.
Details
validate-test [858] SUCCESS. Took 98 min.
Details
@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Jan 26, 2016

awesome, thank you Simon. we'll tout in the M4 release notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment