Fix #1688: don't allow override of synthetic or primitive classes #1689
Conversation
felixmulder
force-pushed
the
topic/fix-override-synth
branch
from
de1fb42 to
e8d3076
Compare
| /** Checks if the user is trying to illegally override synthetic classes, | ||
| * primitives or their boxed equivalent | ||
| */ | ||
| class CheckIllegalOverrides extends MiniPhaseTransform { |
There was a problem hiding this comment.
I wouldn't call this an override, it's more of a re-definition. But I'm also fine with the current name.
| class CheckIllegalOverrides extends MiniPhaseTransform { | ||
| import ast.tpd._ | ||
|
|
||
| override def phaseName = "checkPhantom" |
There was a problem hiding this comment.
It would be nice to have phaseName and class name in sync.
|
|
||
| override def phaseName = "checkPhantom" | ||
|
|
||
| override def transformTypeDef(tree: TypeDef)(implicit ctx: Context, info: TransformerInfo) = { |
There was a problem hiding this comment.
I'd propose to slightly change the way such checks are done.
The problem with this approach is that the other mini-phases in the same block couldn't rely on the facts that you've checking, as they run interleaved with this phase.
As you see, your implementation only needs the symbol and this means that you can make it into an SymTransformer which checks the invariants. Checking in a SymTransformer enforces that even phases in the same mini-phase block that are after you may rely on invariants that you introduce.
| Some("boxed primitive") | ||
| else | ||
| None | ||
|
|
There was a problem hiding this comment.
It would be also nice to check that no-one tries to introduce a companion to those classes.
Eg, there actually is a class Nothing$ that isn't source defined but backend relies on it existing.
There was a problem hiding this comment.
Since we're comparing the full paths of the symbols we get this for free, but I added a neg test i1688c.scala just to make sure.
|
@DarkDimius - addressed your feedback :) |
There was a problem hiding this comment.
IMO, making CheckRedefs a SymTransformer is not the right way to go about it.
- It imposes a tax on every symbol in the whole compilation run. The more SymTransformers are in the pipeline, the slower all transformations get.
- The Symtransformer itself is really slow because it compares fully qualified names.
- It feels like the "one check per phase" system is not workable. The compiler contains on the order of 10^3 checks. Do we really want 10^3 phases?
|
Also, we can't forbid defining primitive value classes here because we do define them when we bootstrap! |
|
I think the correct way to fix #1688 is to change the backend to issue an error and gracefully exit instead of asserting when trying to define |
|
@smarter Or change the frontend to disallow definitions of such classes. Which might prevent other possible crashes. |
|
(Already said this in part on a commit, the emailed link sent me there - reiterating:) @smarter - IIRC when I was looking at this, changing the backend required changing the shared backend that we're depending on from artefact. As @odersky suggested, I could change the
I thought |
Ah, yes, I had not thought of that. But that means a test later on is redundant, right? I.e. if it fires it would be a (weird) internal compiler error. |
|
|
|
IIUC if it fires, it just means that the class that got through with the fully qualified name |
review by: @DarkDimius