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

[2.10.x] assorted fixes for vampire macros #2902

Closed
wants to merge 2 commits into
base: 2.10.x
from

Conversation

Projects
None yet
5 participants
@xeno-by
Member

xeno-by commented Aug 31, 2013

As eloquently elaborated and cleverly named by Travis Brown, macros
defined in structural types are useful:
http://meta.plasm.us/posts/2013/07/12/vampire-methods-for-structural-types/.

However, since such macros are on the intersection of a number of language
features, as usual, there are bugs. This pull requests fixes a couple of them.

no longer warns on calls to vampire macros
As eloquently elaborated and cleverly named by Travis Brown, macros
defined in structural types are useful:
http://meta.plasm.us/posts/2013/07/12/vampire-methods-for-structural-types/.

However, since such macros are on the intersection of a number of language
features, as usual, there are bugs.

This commit fixes an unwanted interaction of macros defined in structural
types with the scala.language.reflectiveCalls guard. Since macro calls
aren't going to be carried to runtime, there's no need to warn about them.
@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by

xeno-by Aug 31, 2013

Member

How come this commit fails mima with:

     [echo] Checking backward binary compatibility for scala-reflect (against 2.10.0)
     [mima] Found 1 binary incompatibiities (40 were filtered out)
     [mima] ======================================================
     [mima]  * method compactifyName(java.lang.String)java.lang.String in trait
     [mima]    scala.reflect.internal.StdNames does not have a correspondent in old version

@retronym @adriaanm @gkossakowski Does this ring a bell? Looks like something spurious...

Member

xeno-by commented on d9a18fb Aug 31, 2013

How come this commit fails mima with:

     [echo] Checking backward binary compatibility for scala-reflect (against 2.10.0)
     [mima] Found 1 binary incompatibiities (40 were filtered out)
     [mima] ======================================================
     [mima]  * method compactifyName(java.lang.String)java.lang.String in trait
     [mima]    scala.reflect.internal.StdNames does not have a correspondent in old version

@retronym @adriaanm @gkossakowski Does this ring a bell? Looks like something spurious...

This comment has been minimized.

Show comment
Hide comment
@gkossakowski

gkossakowski Sep 4, 2013

Member

@xeno-by: it doesn't ring any bell for me...

Member

gkossakowski replied Sep 4, 2013

@xeno-by: it doesn't ring any bell for me...

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Sep 4, 2013

Member

PLS REBUILD/pr-scala@d9a18fb

Member

retronym replied Sep 4, 2013

PLS REBUILD/pr-scala@d9a18fb

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Sep 11, 2013

Member

From the mailing list, @odersky said:

After thinking some more about it, I want to come back to the suggestion of Stefan: Macros can never override but they can overload normal methods. We get the required subtyping between structural types without any further rules that way.

Also, unlike what we thought at the meeting, we need not change the overloading rules to make this work well. In the primary use case of having a foreach macro on Range, that macro already takes precedence over any inherited method in overloading resolution, because it is defined in a subclass of IndexedSeq#foreach.

Finally, with that change, we can drop the rule that prevents a macro from overriding an abstract method, since no overriding is possible in the first place. Everything gets simpler across the board.

Conclusion: I agree with the aims of the pull request, but we should pick a different mechanism to achieve them.

@xeno-by Does this PR need refurbishment in light of Martin's comments?

Member

retronym commented Sep 11, 2013

From the mailing list, @odersky said:

After thinking some more about it, I want to come back to the suggestion of Stefan: Macros can never override but they can overload normal methods. We get the required subtyping between structural types without any further rules that way.

Also, unlike what we thought at the meeting, we need not change the overloading rules to make this work well. In the primary use case of having a foreach macro on Range, that macro already takes precedence over any inherited method in overloading resolution, because it is defined in a subclass of IndexedSeq#foreach.

Finally, with that change, we can drop the rule that prevents a macro from overriding an abstract method, since no overriding is possible in the first place. Everything gets simpler across the board.

Conclusion: I agree with the aims of the pull request, but we should pick a different mechanism to achieve them.

@xeno-by Does this PR need refurbishment in light of Martin's comments?

@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by

xeno-by Sep 11, 2013

Member

@retronym That's what I wanted to discuss yesterday, but we all were so carried away by hygiene :)

I suggest we wait until the next week's reflection meeting.

Member

xeno-by commented Sep 11, 2013

@retronym That's what I wanted to discuss yesterday, but we all were so carried away by hygiene :)

I suggest we wait until the next week's reflection meeting.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Sep 11, 2013

Member

@xeno-by I'll be at the office again today, so we can discuss if you're @ EPFL.

Member

retronym commented Sep 11, 2013

@xeno-by I'll be at the office again today, so we can discuss if you're @ EPFL.

@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by

xeno-by Sep 11, 2013

Member

Don't know whether I'll be at EPFL today - Strange Loop is nearing.

Member

xeno-by commented Sep 11, 2013

Don't know whether I'll be at EPFL today - Strange Loop is nearing.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Sep 11, 2013

Member

No probs. I'll close this PR for now.

Member

retronym commented Sep 11, 2013

No probs. I'll close this PR for now.

@retronym retronym closed this Sep 11, 2013

@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by

xeno-by Sep 24, 2013

Member

Just discussed this at the reflection meeting and decided to reopen as is.

Member

xeno-by commented Sep 24, 2013

Just discussed this at the reflection meeting and decided to reopen as is.

@xeno-by xeno-by reopened this Sep 24, 2013

@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by
Member

xeno-by commented Sep 24, 2013

review @retronym

@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by

xeno-by Sep 24, 2013

Member

Btw would be great to have this in M6.

Member

xeno-by commented Sep 24, 2013

Btw would be great to have this in M6.

@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by

xeno-by Oct 1, 2013

Member

@adriaanm The build is green, but github is still yellow. Is this spurious?

Member

xeno-by commented Oct 1, 2013

@adriaanm The build is green, but github is still yellow. Is this spurious?

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Oct 1, 2013

Member

This is how we encode that an earlier commit failed whereas the last one was fine. Github only looks at the last commit, so it needs to be marked "failed" if any earlier commit failed.

Member

adriaanm commented Oct 1, 2013

This is how we encode that an earlier commit failed whereas the last one was fine. Github only looks at the last commit, so it needs to be marked "failed" if any earlier commit failed.

@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by

xeno-by Oct 1, 2013

Member

PLS REBUILD ALL

Member

xeno-by commented Oct 1, 2013

PLS REBUILD ALL

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

scala-jenkins Oct 1, 2013

(kitty-note-to-self: ignore 25469076)
🐱 Roger! Rebuilding pr-scala for d9a18fb, b779b216. 🚨

scala-jenkins commented Oct 1, 2013

(kitty-note-to-self: ignore 25469076)
🐱 Roger! Rebuilding pr-scala for d9a18fb, b779b216. 🚨

SI-7340 no longer allows to convert vampires to zombies
As eloquently elaborated and cleverly named by Travis Brown, macros
defined in structural types are useful:
http://meta.plasm.us/posts/2013/07/12/vampire-methods-for-structural-types/.

However, since such macros are on the intersection of a number of language
features, as usual, there are bugs.

Before this commit, subtyping checks between structural types didn't
distinguish regular methods structural types (zombies, as per Travis'
classification) and macros (vampires, again as per the same bestiary).

There's one detail though. Since implicit conversions are looked up using
structural types, that look like: "? { def foo: ? }", where question marks
stand for WildcardType, we need to allow such structural types to match
situations when the target member is a macro.

Unlike the original subtyping rule, this one is not a soundness hole,
because: 1) the way the compiler uses such types is guaranteed to be sound,
because they aren't assigned to any term and are discarded right away
after implicit search, 2) users can't actually write such types, so they
can't exploit the situations that lead to unsoundness.
@xeno-by

This comment has been minimized.

Show comment
Hide comment
@xeno-by

xeno-by Oct 4, 2013

Member

Okay since our discussion is going on at #2993, I'm closing this one for the time being.

Member

xeno-by commented Oct 4, 2013

Okay since our discussion is going on at #2993, I'm closing this one for the time being.

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