Simplify type bounds. #2417
Simplify type bounds. #2417
Conversation
|
Review by @retronym |
|
Looks like a winner to me, but I might run it past the Scala meeting on Tuesday to see there are any arguments in defence of the status quo. |
| if (defined.nonEmpty) | ||
| t setPos wrappingPos(defined) | ||
| else | ||
| t setPos o2p(in.offset) |
There was a problem hiding this comment.
What's the difference between start, end and in.offset? I guess in.offset changes, but still why are start and end saved and not used?
There was a problem hiding this comment.
Just a minor nitpick: Is saving three characters worth picking lo/hi over low/high`?
There was a problem hiding this comment.
Or put another way, is adding three characters worth gratuitously making up different names for constructs which already have names? "lo" and "hi" are what they are called.
There was a problem hiding this comment.
start and end are residue from the great deal of flailing it took before I managed to get this tree past the position validator. I'll remove them.
There was a problem hiding this comment.
After losing start and end, the ='s could be aligned further left if there were a 2-letter form for "defined". My brother is an avid Scrabble player and keeps a list of meaningful 2-letter combinations, with surprising results.
There was a problem hiding this comment.
I wonder if that means you don't know about this... http://www.scrabble-assoc.com/ratings/state/co.html
(You have to scroll all the way down to #4.)
There was a problem hiding this comment.
Then there's no excuse.
I would expect all the ='s to line up following seven-letter identifiers.
|
Just to be sure, you still canonicalize empty bounds, but only from |
|
@Blaisorblade This is the parser. It is not performing canonicalization; if you enter "A >: Nothing <: Any" that is what will be encoded. It is simply not inventing bounds. |
I started out looking to limit the noise from empty type
bounds, i.e. the endless repetition of
class A[T >: _root_.scala.Nothing <: _root_.scala.Any]
This led me to be reminded of all the unnecessary and
in fact damaging overreaches which are performed during parsing.
Why should a type parameter for which no bounds are
specified be immediately encoded with this giant tree:
TypeBounds(
Select(Select(Ident(nme.ROOTPKG), tpnme.scala_), tpnme.Nothing),
Select(Select(Ident(nme.ROOTPKG), tpnme.scala_), tpnme.Any)
)
...which must then be manually recognized as empty type bounds?
Truly, this is madness.
- It deftly eliminates the possibility of recognizing
whether the user wrote "class A[T]" or "class A[T >: Nothing]"
or "class A[T <: Any]" or specified both bounds. The fact
that these work out the same internally does not imply the
information should be exterminated even before parsing completes.
- It burdens everyone who must recognize type bounds trees,
such as this author
- It is far less efficient than the obvious encoding
- It offers literally no advantage whatsoever
Encode empty type bounds as
TypeBounds(EmptyTree, EmptyTree)
What could be simpler.
|
@paulp And my point is that whenever you change the parser to canonicalize less equivalent constructs in the source, which is something you want to do, later phases have to be more careful, since they'll have to deal with trees which were impossible before. Probably the extra cost is still worth the more accurate error reporting, but I guess that's not for me to say since I don't know the code well enough. |
|
Oh, my comment from my first version of this PR #2405 didn't make the jump. "This will impact anyone who parses TypeBoundsTrees and never expects to see an EmptyTree in there. I don't know what can be done about that beyond alerting/preparing the usual parties." So I understand it makes trees possible which weren't possible before. |
|
I'm not sure anyone can call the typer a "pure function" with too straight a face. BTW you already had to deal with a potential wide range of untyped trees, which I'm sure nobody did, because they had to look for (at minimum) all of these: And you can't even know if that Nothing is the real Nothing and not some other type called Nothing until it's typed. Not to mention type aliases, etc. etc. etc. You just don't know what the bounds are until it is typed. |
|
OK, so the parser was really not canonicalizing so much in a useful way. |
|
LGTM, and to the Scala meeting today. |
I started out looking to limit the noise from empty type
bounds, i.e. the endless repetition of
This led me to be reminded of all the unnecessary and
in fact damaging overreaches which are performed during parsing.
Why should a type parameter for which no bounds are
specified be immediately encoded with this giant tree:
...which must then be manually recognized as empty type bounds?
Truly, this is madness.
whether the user wrote "class A[T]" or "class A[T >: Nothing]"
or "class A[T <: Any]" or specified both bounds. The fact
that these work out the same internally does not imply the
information should be exterminated even before parsing completes.
such as this author
Encode empty type bounds as
What could be simpler.