Saving symbol lookup from typedIdent. #1521
Conversation
Too many unrelated things are intermingled. If you want to know what symbol corresponds to a particular name in a particular context, you shouldn't have to involve either a Tree or a Typer to find that out. I toiled line by line over typedIdent until it had shed its redundancies and freed itself from the bowels of typed1. The mechanism of name lookup is such that adding a qualifier when the occasion calls for it is inseperable without a lot more effort. So to preserve a sane interface I devised this small partitioning of outcomes. case class LookupSucceeded(qualifier, symbol) case class LookupAmbiguous(msg) case class LookupInaccessible(symbol, msg) case class LookupNotFound(msg)
This unifies several disparate/ad-hoc mechanisms for excluding symbols from eligibility in a single predicate. This is the method on Context: def lookupSymbol(name: Name, qualifies: Symbol => Boolean) The logic is largely that which was buried in typedIdent, except that I fixed SI-3160 so that import foo._ does not inject foo's private members into your namespace.
Given that it's just a reimplementation of Option, we may as well not also reimplement methods like map and getOrElse at every call site.
Greasing the wheels for Typer's well-being.
Apply convenience methods to strip away complications.
This completes the transition. Typer's bevy of special cases to influence symbol lookup are encoded in its local "qualifies" method, which it passes to lookupSymbol. This allows access to be done correctly without infecting Typer with such pedestrian concerns.
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/770/ |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1481/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1481/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/770/ |
|
Just for FYI: I was about test those changes for performance but I realized that before leaving Lausanne I was doing some refactoring of my performance infrastructure so I'd get better accuracy for my benchmarks. I changed how data is being produced but not how it's being analyzed. I need to fix it it will take a while :-( |
|
Take all the time you need, as long as you won't stand in the way of progress. This gives every sign of being better in terms of performance, and that's its least attractive feature. |
|
Has it really only been 5 days? Feels like 50. The amount of work involved in a patch like this is obscene. (Nothing like a few undiscovered pattern matcher bugs to liven things up, too.) I would like to merge it. |
|
It wasn't a pattern matcher bug. I added a comment about why it failed last week: #1521 (comment) |
|
It was a pattern matcher bug in the ordinary sense that a) there was a pattern and b) I tried to match with it and c) it didn't do the right thing. "pattern matcher bug" is not to my knowledge an appellation which requires a formal entrance exam. |
|
i believe the matcher's behaviour is as spec'ed. Those objects are actually different because the outer pointers are different. |
|
the cast I quoted above, |
|
Okay, then why does the case class match? Shouldn't d.LookupNotFound() and localTyper.LookupNotFound() also be from different fruit bins? |
|
I'm going to say because the outer pointer cannot be tested, but it's a good question and I should find out. |
|
the green bytecode from #1521 (comment) seems to support my hypothesis Trying to figure out why the fruits are on different trees now. |
|
When can and cannot the outer pointer be tested, exactly? Should I now expect that moving a block of code between files without changing it or touching the visible semantics might start cases to start matching or stop matching? Have we considered what a bad idea this is? Proposal: warn 100% of the time if a case would have matched, but does not due to the outer pointer check. Frequency with which this warning will be spurious noise: 0.1%. Frequency with which it will catch deadly silent not-matches where it was never intended that there be multiple "families" of some class: 99.9%. (I would like to meet the programmers for whom the outer pointer test is an intentional element of their design upon which correctness depends... because I have seen the old pattern matcher from the inside.) |
Turns out putting a group of case classes at what feels like the top level might not be top-level enough, like if your "top" is Analyzer and you wind up with different outer pointers in every instance of Typer. Moved the whole bundle to SymbolTable.
|
Well, testing outer pointers has always been part of the spec. It was poorly implemented, but it is required for type soundness. Types selected on different prefixes should not be treated equal. It is unfortunate that we cannot always test outer pointers (which was recently made harder by #954). |
|
That said, the type system will not even let you compare those classes unless you sneakily cast the prefix to something it is not. |
|
Down comes the house of cards! https://issues.scala-lang.org/browse/SI-6583 |
|
PLS REBUILD ALL (it seems it wasn't triggered on the last commit -- just to be sure...) |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/816/ |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1525/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1525/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/816/ |
Saving symbol lookup from typedIdent.
Anticipating the appeals to performance, I offer this coarse measurement. My times are the good ones. Better measurements are welcome, but I convinced myself at least that I wasn't hurting anything.
Review by @odersky, @adriaanm.