Skip to content

Make the cached mirror methods invisible #13825

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Make the cached mirror methods invisible #13825

wants to merge 4 commits into from

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Oct 26, 2021

Fixes #13815

@odersky
Make the fromProduct method invisible
cb92fba
@odersky
Avoid confusion in "- did you mean" messages
4adc2cd 
Sometimes the selected member exists but is not visible. In that case we should
not issue a "did you mean" message with exactly the same name as what was written.
@odersky odersky requested a review from bishabosha November 3, 2021 08:47
@odersky odersky assigned bishabosha and unassigned odersky Nov 3, 2021
bishabosha
bishabosha requested changes Nov 3, 2021
View reviewed changes
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.

the ordinal method should also be made invisible:

// Shape_1.scala
sealed trait Shape

object Shape:
  case class Circle(r: Int) extends Shape
  case class Square(width: Int) extends Shape
// Test_2.scala
val circleOrdinal = Shape.ordinal(Shape.Circle(r = 23))

@bishabosha bishabosha assigned odersky and unassigned bishabosha Nov 3, 2021
@odersky odersky assigned bishabosha and unassigned odersky Nov 3, 2021
@bishabosha bishabosha self-requested a review November 3, 2021 10:38
@bishabosha bishabosha changed the title Make the fromProduct method invisible Make the cached mirror methods invisible Nov 3, 2021
bishabosha
bishabosha requested changes Nov 3, 2021
View reviewed changes
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.

Just thinking now that the MirroredMonoType is also visible too

@odersky
Copy link
Contributor Author

odersky commented Nov 3, 2021

Right. Fix is forthcoming.

@bishabosha
Copy link
Member

however I have thought of something else, which is that the companion will still extend Mirror.Sum or Mirror.Product - so you can widen the type to call the method:

4 |val circleOrdinal = (Shape: deriving.Mirror.Sum).ordinal(Shape.Circle(r = 23)) // error
  |                                                         ^^^^^^^^^^^^^^^^^^^^
  |              Found:    Shape.Circle
  |              Required: ?1.MirroredMonoType
  |
  |              where:    ?1 is an unknown value of type deriving.Mirror.Sum

could there be a way to make a parent invisible?

bishabosha
bishabosha reviewed Nov 3, 2021
View reviewed changes
tests/neg/i13815.check
| ^^^^^^^^^^^^^
| value ordinal is not a member of object Shape
| ^^^^^^^^^^^^^^^^^^^^
| Found: Shape.Circle
Copy link
Member

Choose a reason for hiding this comment

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

this seems strange that making MirroredMonoType invisible now makes ordinal visible again

@bishabosha
Copy link
Member

bishabosha commented Nov 3, 2021 •
edited
Loading

maybe this needs to be solved on the unpickling side - ignore the mirror parent in tasty (if it has invisible/synthetic mirror methods), then in posttyper re-add the parent

@odersky
Copy link
Contributor Author

odersky commented Nov 3, 2021

maybe this needs to be solved on the unpickling side - ignore the mirror parent in tasty (if it has invisible/synthetic mirror methods), then in posttyper re-add the parent

I believe that would be a hole of complexity.

@odersky
Copy link
Contributor Author

odersky commented Nov 3, 2021

could there be a way to make a parent invisible?

I don't think so. I don't really want to sink more time into this. It is what it is.

@odersky
Copy link
Contributor Author

odersky commented Nov 3, 2021

I think we should close this. I don't see a good way to address the issue currently.

@odersky odersky closed this Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bishabosha bishabosha bishabosha requested changes

Assignees

@bishabosha bishabosha

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

synthetic Mirror methods become visible when from classpath dependency
2 participants
@odersky @bishabosha