RFR: JDK-8205418: Assorted improvements to source code model

[Maurizio Cimadamore]

Hi Jan,

* in Types and Operators, I wonder if using 'xyz == Type.recoveryType' 
is the right thing - and we wouldn't be more general by doing 
xyz.hasTag(NONE), as in other functions. I mean, is the tweaking you did 
for the castable function something that should only happen if the type 
involved in a cast has to do with recovery? Or, more generally, we want 
just to skip spurious error generation when a type info isn't there? I 
think the latter might be better?

* RecoveryInfo - why was the report() method changed not to report 
errors? Was this generating spurious messages? Now that, with your 
change, some recoveryInfo _do_ have expected type info associated with 
them - is it still the case that we should always have 'compatible' 
return true, (and not emit error messages) ? Also, nit, I'd prefer 
adding a comment in the empty body saying 'do nothing' e.g. showing it's 
a deliberate choice (we do that elsewhere).

* Resolve - not sure I get the bestCandidate part. I mean, it seems you 
want to check that all candidates point to the same symbol. But you 
don't seem to look as to whether the candidate was applicable or not 
(and the phase at which it was not applicable is discarded too). Sure 
that errCandidate() isn't what you want here? errCandidate picks the 
last non-applicable candidate (which has the higher resolution phase). 
Note that when an InapplicableSymbolError is issued, there should always 
be just _one_ inapplicable method, otherwise you'd get an 
InapplicableSymbol_s_Error. So I'm not sure why you have that logic to 
check that all candidates are the same.

* DeferredAttr - seems that at least some of the hoops you have to jump 
through to enforce an expected type with the mapping could be avoided if 
you made the mapper accept a Type visitor argument (currently, 
DeferredMap has a Void type argument).

* Resolve - I believe what your changes want to do here is: if we issued 
an ambiguous error, then we shouldn't have

* T8071432.out - I'd say the fact that one error is missing here is step 
backwards? Is this caused by the removal of logging in the recovery info?

* IgnoreLambdaBodyDuringResolutionTest2.out - this time the removal of 
the diagnostic seems more benign

As for whether this should go in 11 or 12, I guess I don't have a strong 
opinion. Some of the things you touch are sensitive, as Vicente 
mentioned, but at the same time I can't see obvious ways as to why they 
should impact correctness - but they will almost surely have some 
non-trivial consequences on compiler diagnostics - which might even 
perceived as regressions (see above).

Maurizio


On 21/06/18 12:43, Jan Lahoda wrote:
> Hi,
>
> I'd like to propose a few improvements to the source code model as 
> seen through the com.sun.source APIs.
>
> The changes include better error recovery, a fix end positions, and a 
> fix for Trees.getScope when used with a TreePath that point to a block 
> of a lambda expression, including cases where there's an error in the 
> source code.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8205418
> Webrev: http://cr.openjdk.java.net/~jlahoda/8205418/webrev.00/
>
> How does this look?
>
> Thanks,
>    Jan