Skip to content

Change structural types scheme #9180

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 7 commits into from
Jun 18, 2020
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jun 14, 2020

Make Selectable a marker trait without any members.

@odersky odersky marked this pull request as draft June 14, 2020 20:38
@odersky
Copy link
Contributor Author

odersky commented Jun 16, 2020

I reverted the previous attempt to use import language.reflectiveCalls as an alternative way to import the reflectiveSelectable conversion. It caused ambiguities. I briefly considered to make
language.reflectiveCalls the only way to produce a reflective.Selectable, but decided against it. Doing this would be a useful hack in the short run, but would destroy all invariants we have for the language` module: Namely, that it is compile-time only and that every entry is mirrored by a command-line option.

@odersky odersky marked this pull request as ready for review June 17, 2020 11:34
@odersky odersky requested a review from sjrd June 17, 2020 11:56
Copy link
Member

@sjrd sjrd 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 @odersky. I have some small comments. The implementation looks good, but I really think we should have a test with contextual parameters in the selectDynamic and applyDynamic methods, which was at least half of the point of this new scheme.

@sjrd sjrd assigned odersky and unassigned sjrd Jun 17, 2020
odersky added 2 commits June 18, 2020 10:03
Make Selectable a marker trait without any members.
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

There's a SemanticDB test that failed in bootstrap:

[info] Test dotty.tools.dotc.semanticdb.SemanticdbTests.expectTests started
[error] check file /__w/dotty/dotty/tests/semanticdb/expect/Advanced.expect.scala does not match generated.
If you meant to make a change, replace the expect file by:
  mv /__w/dotty/dotty/tests/semanticdb/expect/Advanced.expect.scala.out /__w/dotty/dotty/tests/semanticdb/expect/Advanced.expect.scala
inspect with:
  diff /__w/dotty/dotty/tests/semanticdb/expect/Advanced.expect.scala /__w/dotty/dotty/tests/semanticdb/expect/Advanced.expect.scala.out
Or else update all expect files with
  sbt 'dotty-compiler-bootstrapped/test:runMain dotty.tools.dotc.semanticdb.updateExpect'
[error] Test dotty.tools.dotc.semanticdb.SemanticdbTests.expectTests failed: java.lang.AssertionError: 1 errors in expect test., took 1.397 sec
[error]     at dotty.tools.dotc.semanticdb.SemanticdbTests.runExpectTest(SemanticdbTests.scala:82)
[error]     at dotty.tools.dotc.semanticdb.SemanticdbTests.expectTests(SemanticdbTests.scala:39)
[error]     ...
[info] Test run finished: 1 failed, 0 ignored, 1 total, 1.398s

but otherwise LGTM :)

@odersky odersky merged commit ebe223e into scala:master Jun 18, 2020
@odersky odersky deleted the structural-new branch June 18, 2020 12:33
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.

2 participants