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

[Jan Lahoda]

Hi Maurizio,

Thanks for the comments.

On 22.6.2018 11:53, Maurizio Cimadamore wrote:
> 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?

NONE sounds good as well, will change that.

>
> * RecoveryInfo - why was the report() method changed not to report
> errors? Was this generating spurious messages? Now that, with your

Yes, there were some extra error messages (that didn't seem very useful).

> 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

I think (hope) not emitting the error messages is fine - the intent here 
is only to fill the model with some (hopefully) meaningful data.

> adding a comment in the empty body saying 'do nothing' e.g. showing it's
> a deliberate choice (we do that elsewhere).

Sure, will do (unless this change is undone).

>
> * 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.

InapplicableSymbol_s_Error extends from InapplicableSymbolError, right? 
So there may be multiple inapplicable symbols, I think. The intent is 
here to try to recover in case where the expected symbol is obvious, but 
to not a pick a candidate if there's any ambiguity possible.

>
> * 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).

Thanks, will look into that.

>
> * 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?

For default/simplified errors, there is only one error message that is 
the same before and after this patch:
---
T8071432.java:47: error: incompatible types: cannot infer type-variable(s) U
         System.out.println(c.stream().reduce(0.0,
                                             ^
     (argument mismatch; bad return type in lambda expression
       int cannot be converted to Double)
   where U,T are type-variables:
     U extends Object declared in method <U>reduce(U,BiFunction<U,? 
super T,U>,BinaryOperator<U>)
     T extends Object declared in interface Stream
Note: Some messages have been simplified; recompile with -Xdiags:verbose 
to get full output
1 error
---

But it is true that for -Xdiags:verbose, there were two errors, and with 
this patch there is only one, and the error that is missing is probably 
the more useful one. I'll investigate what are the options.

Thanks for the comments!

Jan

>
> * 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
>