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

Fix #7991: don't set JavaDefined for Dotty Enum module class #8008

Merged
merged 13 commits into from Jan 17, 2020

Conversation

@liufengyun
Copy link
Contributor

liufengyun commented Jan 15, 2020

Fix #7991: don't set JavaDefined for Dotty Enum module class

Top-level Dotty Enum classes have the flag JAVA_ACC_ENUM.
We cannot tell from the flag whether a class is JavaDefined or not.
The moduleRoot already has the flag JavaDefined set, it suffices
to test the flag.

liufengyun added 2 commits Jan 15, 2020
Top-level Dotty Enum classes have the flag JAVA_ACC_ENUM.
We cannot tell from the flag whether a class is JavaDefined or not.
The `moduleRoot` already has the flag `JavaDefined` set, it suffices
to test the flag.
@smarter

This comment has been minimized.

Copy link
Member

smarter commented Jan 15, 2020

#7501 lists other NoSuchFieldError issues which should be fixed by this

Copy link
Member

smarter left a comment

I haven't checked but it's possible this also fixes the "Bad type operand on stack" issues mentioned in #7501

@smarter

This comment has been minimized.

Copy link
Member

smarter commented Jan 15, 2020

Ah, I see you already added tests for them :).

@bishabosha

This comment has been minimized.

Copy link
Member

bishabosha commented Jan 16, 2020

for #7410 can we also test a statement in the REPL accessing an enum constant from the class path

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Jan 16, 2020

We should also have a variant of tests/run/i7991 where the Num is a java enum. That may be what is currently failing with the error message IDs enum.

liufengyun added 5 commits Jan 16, 2020
Moving definitions will make Scala code crash at runtime.
See `tests/run/i6664b` for an example.
Currently the only way to put initializer inside
the static constructor is to use @static. However, CheckStatic
restricts that @static can only used in companion object, which
is a user-facing check, as MoveStatics can handle @static member
in the class.

We need a refactoring to make @static a general mechanism that
includes a phase to generate the static constructor. This needs
to be addressed in another PR.
new CompleteJavaEnums) :: // Fill in constructors for Java enums
List(new CheckStatic, // Check restrictions that apply to @static members
new CheckStatic) :: // Check restrictions that apply to @static members
List(new CompleteJavaEnums, // Fill in constructors for Java enums

This comment has been minimized.

Copy link
@anatoliykmetyuk

anatoliykmetyuk Jan 17, 2020

Contributor

Why is this swap needed? Since we are adding the @static annotation in CompleteJavaEnums, should this phase not go before CheckStatic so that the latter can check the newly added static members?

This comment has been minimized.

Copy link
@liufengyun

liufengyun Jan 17, 2020

Author Contributor

CheckStatic will reject our code, because it requires that a @static member is located in a companion object, not the class.

This comment has been minimized.

Copy link
@anatoliykmetyuk

anatoliykmetyuk Jan 17, 2020

Contributor

Would it be a better idea to modify CheckStatic to accept the code? E.g. (from CheckStatic doc), "1. Only objects and those classes which extend java.lang.Enum can have members annotated with @static"?

The meaning of CheckStatic is to provide guarantees on @static. If there exists code that doesn't follow the existing guarantees, this complicates the codebase maintenance and usage. We have the exact same scenario with setting flags on completion: there are rules about which flags are immutable wrt completion but they are not enforced. And hence there are lots of places in the codebase that don't follow these rules.

This comment has been minimized.

Copy link
@liufengyun

liufengyun Jan 17, 2020

Author Contributor

I have a commit message in f6f3b28: the @static infrastructure needs fundamental refactoring, but it's out of the scope of this PR.

Copy link
Contributor

anatoliykmetyuk left a comment

LGTM, since CheckStatic is a user-facing check, in the sense that it is a very early phase and lots of other phases work without these guarantees, I think it's fine to also not have them for the enums phase.

@anatoliykmetyuk anatoliykmetyuk merged commit 3e08142 into lampepfl:master Jan 17, 2020
2 checks passed
2 checks passed
CLA User signed CLA
Details
continuous-integration/drone/pr Build is passing
Details
@anatoliykmetyuk anatoliykmetyuk deleted the dotty-staging:fix-7991 branch Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.