RFR: JDK-8187950: javax.lang.model APIs throws CompletionFailure or a subtype of CompletionFailure.

[Liam Miller-Cushon]

I did some more testing and found a handful of annotation processors
affected by the change. For the most part they were already checking for
error types, and adding some missing error handling fixed them. So from
what I saw the compatibility impact is fairly minor.

I still think there might be advantages to handling these errors with
exceptions instead of error types, and it might be interesting to consider
ways to add that to the API (perhaps as optional behaviour for processors
that want it, to preserve compatibility). But as you said, this change
makes the existing behaviour more consistent and brings it in line with the
current spec, both of which are good things.

So, this looks good to me. Thanks for the discussion!

On Mon, Feb 19, 2018 at 10:57 AM, Jan Lahoda <jan.lahoda at oracle.com> wrote:

> Hi,
>
> I was testing the webrev.01-ext, and so far it seems OK, so I added that
> to the patch.
>
> One thing I've noticed is that com.sun.source.tree.Scope.getLocalElements()
> may still cause the CompletionFailure to be thrown to the client code, so
> I've fixed that.
>
> Updated webrev:
> http://cr.openjdk.java.net/~jlahoda/8187950/webrev.02/
>
> There are two bugs related to this (JDK-8177068 and JDK-8198378, both
> already existing) on which I plan to work separately.
>
> Are there any comments/feedback on the patch?
>
> Thanks,
>     Jan
>
>
> On 14.2.2018 18:58, Jan Lahoda wrote:
>
>> On 14.2.2018 18:04, Liam Miller-Cushon wrote:
>>
>>> On Wed, Feb 14, 2018 at 7:33 AM, Jan Lahoda <jan.lahoda at oracle.com
>>> <mailto:jan.lahoda at oracle.com>> wrote:
>>>
>>>     Today, I got an idea that might solve this issue without too much
>>>     hassle, sketched here:
>>>     http://cr.openjdk.java.net/~jlahoda/8187950/webrev.01-ext/
>>>     <http://cr.openjdk.java.net/~jlahoda/8187950/webrev.01-ext/>
>>>
>>>     Will need more testing, but possibly could work. Any opinions/ideas?
>>>
>>>
>>> Thanks - that fixed the first two regressions I saw. I'm not sure
>>> whether getKind() was the only instance of this problem, but I'm running
>>> more tests and will report back.
>>>
>>
>> Cool, thanks!
>>
>>
>>>     One thing I'd like to point out is that this can (AFAIK) happen even
>>>     now - if the interface B is completed in a way that will throw away
>>>     the CompletionFailure (e.g. Elements.getTypeElement), then the
>>>     subsequent code will AFAIK see the same state as this AP sees. So it
>>>     seemed somewhat acceptable to let the behavior be similar in case
>>>     where the CompletionFailure would be thrown, esp. since I didn't see
>>>     a good way to do better.
>>>
>>>
>>> Good point. I guess I've seen more issues around processors catching
>>> completion failures and leaving the model in a bad state for other
>>> processors (or javac), than around the completion failure itself causing
>>> problems. It's important that processors can detect when they're seeing
>>> invalid input. If they end up needing to be more vigilant about checking
>>>
>>
>> FWIW, in the current state, it is possible to check
>> DeclaredType.asElement().asType().getKind() == TypeKind.ERROR to see if
>> the type's symbol is broken. (I agree it is quite cumbersome.)
>>
>> for error types that's a slightly incompatible change.
>>>
>>
>> AFAIK, doing TypeElement.getSuperclass() may currently lead to several
>> results depending on the exact circumstances:
>> 1) a DECLARED type, that will throw a CompletionFailure (once) at some
>> point if used
>> 2) a DECLARED type that will not throw a CompletionFailure (because it
>> was already thrown)
>> 3) an ERROR type (if the TypeElement originates in the source code, and
>> its superclass is missing)
>>
>> So eliminating any of these is probably a slightly incompatible change
>> (which will need a CSR), but I personally think it is better than having
>> different behaviors. OTOH, I suspect many APs already deal with these in
>> some ways, so eliminating some shouldn't hopefully be that disruptive.
>>
>>
>>> There was some discussion in JDK-8187950 about declaring an exception in
>>> the API contract for methods that currently throw completion failures.
>>> Did you consider taking that approach, and having the completion failure
>>> be rethrown to ensure other processors and javac see it if they try to
>>> complete the same symbol? Maybe JDK-8190054 answers that question - it
>>> sounds like there's a preference for returning error objects instead of
>>> throwing?
>>>
>>
>> I was thinking of that, but I personally:
>> -think the model is cleaner without the exceptions
>> -am not convinced that it would be much simpler if we tried to change
>> the implementation to more consistently throw the exception
>>
>> Thanks for your feedback,
>>      Jan
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20180220/ce8c0874/attachment.html>