Skip to content

Consider module accessor a JSGetter #2541

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

Merged
merged 1 commit into from
Jul 21, 2016

Conversation

adriaanm
Copy link
Contributor

@adriaanm adriaanm commented Jul 20, 2016

Redundant on 2.11, but required for the upcoming 2.12.0-RC1,
where the field refactoring synthesizes module accessors
after uncurry, so that their type history does not even
go back as far as uncurry, where a NullaryMethodType might
have been observed otherwise.

I tried splicing NMTs in the info history of modules,
but they already are pretty hacking (a module's term symbol
is converted into a method pretty late in the game, and it
would be risky to have them start with NMTs from namers).

With this, scala-js's test suite passes on scala/scala#5141
(well, I'm about to rewind that branch to a stable point where this holds)

TODO

  • add comment

@scala-jenkins
Copy link

Can one of the admins verify this patch?

@@ -108,13 +108,13 @@ trait JSGlobalAddons extends JSDefinitions

/** has this symbol to be translated into a JS getter (both directions)? */
def isJSGetter(sym: Symbol): Boolean = {
sym.tpe.params.isEmpty && enteringUncurryIfAtPhaseAfter {
sym.isModule || (sym.tpe.params.isEmpty && enteringUncurryIfAtPhaseAfter {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we only get here when sym.isMethod, thus sym.isModule can only hold for the module's accessor

Copy link
Member

Choose a reason for hiding this comment

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

Could you write that as a comment in the code?

See https://github.com/scala-js/scala-js/blob/master/CODINGSTYLE.md#non-scaladoc-comments for the style we use for comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do tomorrow!

adriaanm added a commit to adriaanm/community-builds that referenced this pull request Jul 21, 2016
@sjrd
Copy link
Member

sjrd commented Jul 21, 2016

Jenkins, test this please

@sjrd
Copy link
Member

sjrd commented Jul 21, 2016

That's all. Thanks!

Redundant on 2.11, but required for the upcoming 2.12.0-RC1,
where the field refactoring synthesizes module accessors
after uncurry, so that their type history does not even
go back as far as uncurry, where a NullaryMethodType might
have been observed otherwise.

I tried splicing NMTs in the info history of modules,
but they already are pretty hacking (a module's term symbol
is converted into a method pretty late in the game, and it
would be risky to have them start with NMTs from namers).
@adriaanm adriaanm force-pushed the 2.12-fields-compat branch from 80d9829 to 21e499c Compare July 21, 2016 05:08
@adriaanm
Copy link
Contributor Author

Amended my commit.

adriaanm added a commit to adriaanm/community-builds that referenced this pull request Jul 21, 2016
@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3311/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4309/
Test PASSed.

@sjrd
Copy link
Member

sjrd commented Jul 21, 2016

LGTM Jenkins, retest this please

@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3316/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4314/
Test PASSed.

@sjrd sjrd merged commit 65d53c9 into scala-js:master Jul 21, 2016
@adriaanm adriaanm deleted the 2.12-fields-compat branch August 10, 2016 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants