Interim JDK 10 RFR of 8187951: Update javax.lang.model.SourceVersion for "var" name

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Interim JDK 10 RFR of 8187951: Update javax.lang.model.SourceVersion for "var" name

joe darcy
Hello,

Please review the in-progress work on

     8187951: Update javax.lang.model.SourceVersion for "var" name
     http://cr.openjdk.java.net/~darcy/8187951.0/

I'm not 100% certain of the desired semantics here. The handling of
"var" doesn't align with any of the existing syntactic constructs,
including restricted keywords introduced in 9 with modules.

The proposal changes gives the "isName" method the semantics of "can I
use this qualified name for a type, field, package, or module."

Thanks,

-Joe

Reply | Threaded
Open this post in threaded view
|

Re: Interim JDK 10 RFR of 8187951: Update javax.lang.model.SourceVersion for "var" name

Maurizio Cimadamore

Not 100% sure about this. On the one hand, the concept of contextual keyword is, well, associated with a context. So 'open' is a keyword in a module-info file, and 'var' is a keyword when used in a local variable declaration.

But the isName method takes no such context, so it's bound to be lossy.

You tried to reformulate the semantics of isName as something similar to 'is this a valid name for a javax.lang.model.element.Element instance'? I think that _almost_ works - after all elements model:

* variables/fields (VariableElement)
* methods (ExecutableElement)
* classes/interfaces/enums/annotations (TypeElement)
* typevars (TypeParameterElement)
* modules (ModuleElement)
* packages (PackageElement)

now, in the last two cases, I'm not sure the implemented logic works well. While 'var' is not a legal name for variables/methods/types/typevars, I think it is a valid name for packages and modules, there's no restriction there.

This unfortunately makes the definition of isName messy, because it doesn't really refer to _all_ elements, but just to a subset of them.

Also, I find this text:

"and {@code "var"} is used for local variable type inference in the argument version."

I only got a good idea of what you meant by looking at the implementation.

One alternative would be to mark this method as @Deprecated, and add a new method:

public static boolean isName(CharSequence name, ElementKind kind, SourceVersion version) { ... }

Where the semantics could be:

is _name_ a valid qualified name for an element of kind _kind_ in version _version_ ?

While not perfect (it still cannot handle module-info keyword, as there's no info on which compilation unit this question refers to), I think it's a step forward in that it lets you discriminate between different kind of elements.

An even more complete variant would be:

public static boolean isName(CharSequence name, JavaFileObject sourceFile, ElementKind kind, SourceVersion version) { ... }

Which would let you express in full what you need to express, for both 'var' and module keywords.

Maurizio

On 27/09/17 23:51, joe darcy wrote:
Hello,

Please review the in-progress work on

    8187951: Update javax.lang.model.SourceVersion for "var" name
    http://cr.openjdk.java.net/~darcy/8187951.0/

I'm not 100% certain of the desired semantics here. The handling of "var" doesn't align with any of the existing syntactic constructs, including restricted keywords introduced in 9 with modules.

The proposal changes gives the "isName" method the semantics of "can I use this qualified name for a type, field, package, or module."

Thanks,

-Joe


Reply | Threaded
Open this post in threaded view
|

Re: Interim JDK 10 RFR of 8187951: Update javax.lang.model.SourceVersion for "var" name

joe darcy

Hi Maurizio,

I agree that implementing a more precise version of the can-I-use-this-as-a-name check requires more context, such as the element kind as you suggest.

My sense is that additional API scope isn't warranted. Within the existing set of methods, I think having isName and isKeyword both return "false" for var for RELEASE_10 and later would be acceptable.

What to you think?

Thanks,

-Joe


On 9/28/2017 1:35 AM, Maurizio Cimadamore wrote:

Not 100% sure about this. On the one hand, the concept of contextual keyword is, well, associated with a context. So 'open' is a keyword in a module-info file, and 'var' is a keyword when used in a local variable declaration.

But the isName method takes no such context, so it's bound to be lossy.

You tried to reformulate the semantics of isName as something similar to 'is this a valid name for a javax.lang.model.element.Element instance'? I think that _almost_ works - after all elements model:

* variables/fields (VariableElement)
* methods (ExecutableElement)
* classes/interfaces/enums/annotations (TypeElement)
* typevars (TypeParameterElement)
* modules (ModuleElement)
* packages (PackageElement)

now, in the last two cases, I'm not sure the implemented logic works well. While 'var' is not a legal name for variables/methods/types/typevars, I think it is a valid name for packages and modules, there's no restriction there.

This unfortunately makes the definition of isName messy, because it doesn't really refer to _all_ elements, but just to a subset of them.

Also, I find this text:

"and {@code "var"} is used for local variable type inference in the argument version."

I only got a good idea of what you meant by looking at the implementation.

One alternative would be to mark this method as @Deprecated, and add a new method:

public static boolean isName(CharSequence name, ElementKind kind, SourceVersion version) { ... }

Where the semantics could be:

is _name_ a valid qualified name for an element of kind _kind_ in version _version_ ?

While not perfect (it still cannot handle module-info keyword, as there's no info on which compilation unit this question refers to), I think it's a step forward in that it lets you discriminate between different kind of elements.

An even more complete variant would be:

public static boolean isName(CharSequence name, JavaFileObject sourceFile, ElementKind kind, SourceVersion version) { ... }

Which would let you express in full what you need to express, for both 'var' and module keywords.

Maurizio

On 27/09/17 23:51, joe darcy wrote:
Hello,

Please review the in-progress work on

    8187951: Update javax.lang.model.SourceVersion for "var" name
    http://cr.openjdk.java.net/~darcy/8187951.0/

I'm not 100% certain of the desired semantics here. The handling of "var" doesn't align with any of the existing syntactic constructs, including restricted keywords introduced in 9 with modules.

The proposal changes gives the "isName" method the semantics of "can I use this qualified name for a type, field, package, or module."

Thanks,

-Joe



Reply | Threaded
Open this post in threaded view
|

Re: Interim JDK 10 RFR of 8187951: Update javax.lang.model.SourceVersion for "var" name

Jonathan Gibbons

Joe,

I guess I would ask, who are these methods targeted at?  

Both isIdentifier and isKeyword are fuzzy at best. 

isIdentifier does not take the appropriate Unicode standard into account, although it is probably "good enough".

isKeyword has certainly become more complex in 9 and after, and anyone who really needs this info (e.g. IDEs) is probably already doing it for themselves, properly.

Do you have any reasonable use-cases for a fuzzy definition of isKeyword, or should we just deprecate it words the Big Bit Bucket in the sky?

-- Jon



On 9/29/17 10:41 AM, joe darcy wrote:

Hi Maurizio,

I agree that implementing a more precise version of the can-I-use-this-as-a-name check requires more context, such as the element kind as you suggest.

My sense is that additional API scope isn't warranted. Within the existing set of methods, I think having isName and isKeyword both return "false" for var for RELEASE_10 and later would be acceptable.

What to you think?

Thanks,

-Joe


On 9/28/2017 1:35 AM, Maurizio Cimadamore wrote:

Not 100% sure about this. On the one hand, the concept of contextual keyword is, well, associated with a context. So 'open' is a keyword in a module-info file, and 'var' is a keyword when used in a local variable declaration.

But the isName method takes no such context, so it's bound to be lossy.

You tried to reformulate the semantics of isName as something similar to 'is this a valid name for a javax.lang.model.element.Element instance'? I think that _almost_ works - after all elements model:

* variables/fields (VariableElement)
* methods (ExecutableElement)
* classes/interfaces/enums/annotations (TypeElement)
* typevars (TypeParameterElement)
* modules (ModuleElement)
* packages (PackageElement)

now, in the last two cases, I'm not sure the implemented logic works well. While 'var' is not a legal name for variables/methods/types/typevars, I think it is a valid name for packages and modules, there's no restriction there.

This unfortunately makes the definition of isName messy, because it doesn't really refer to _all_ elements, but just to a subset of them.

Also, I find this text:

"and {@code "var"} is used for local variable type inference in the argument version."

I only got a good idea of what you meant by looking at the implementation.

One alternative would be to mark this method as @Deprecated, and add a new method:

public static boolean isName(CharSequence name, ElementKind kind, SourceVersion version) { ... }

Where the semantics could be:

is _name_ a valid qualified name for an element of kind _kind_ in version _version_ ?

While not perfect (it still cannot handle module-info keyword, as there's no info on which compilation unit this question refers to), I think it's a step forward in that it lets you discriminate between different kind of elements.

An even more complete variant would be:

public static boolean isName(CharSequence name, JavaFileObject sourceFile, ElementKind kind, SourceVersion version) { ... }

Which would let you express in full what you need to express, for both 'var' and module keywords.

Maurizio

On 27/09/17 23:51, joe darcy wrote:
Hello,

Please review the in-progress work on

    8187951: Update javax.lang.model.SourceVersion for "var" name
    http://cr.openjdk.java.net/~darcy/8187951.0/

I'm not 100% certain of the desired semantics here. The handling of "var" doesn't align with any of the existing syntactic constructs, including restricted keywords introduced in 9 with modules.

The proposal changes gives the "isName" method the semantics of "can I use this qualified name for a type, field, package, or module."

Thanks,

-Joe




Reply | Threaded
Open this post in threaded view
|

Re: Interim JDK 10 RFR of 8187951: Update javax.lang.model.SourceVersion for "var" name

joe darcy
Hi Jon,

Catching up post-JavaOne...

On 9/29/2017 11:12 AM, Jonathan Gibbons wrote:

Joe,

I guess I would ask, who are these methods targeted at?  

Both isIdentifier and isKeyword are fuzzy at best. 

isIdentifier does not take the appropriate Unicode standard into account, although it is probably "good enough".


Right; specifically it doesn't use the Unicode standard that was in force at the time of the earlier releases. Each major JDK platform release generally bumps the supported Unicode version and set of identifier characters expands. To be 100% accurate the isIdentifier check on release (N - k) for some k > 0 would need the Unicode tables used on JDK (N - k). That information is not available via the platform APIs.

isKeyword has certainly become more complex in 9 and after, and anyone who really needs this info (e.g. IDEs) is probably already doing it for themselves, properly.

Do you have any reasonable use-cases for a fuzzy definition of isKeyword, or should we just deprecate it words the Big Bit Bucket in the sky?


My thought was for annotation processor authors to have some easy ways to verify a name was valid to use on a given source version, etc.

I agree tools like IDEs will need more precise implementations of such checks, but I think there is still benefit in providing less formal checking too.

-Joe

Reply | Threaded
Open this post in threaded view
|

Re: Interim JDK 10 RFR of 8187951: Update javax.lang.model.SourceVersion for "var" name

Volker Simonis
Hi Joe,

I just have a quick question regarding the overall development
approach for jdk 10 (aka jdk 18.3).

As Mark has wrote several times (e.g. [1]) "every feature must be
complete before it's integrated". Your change clearly belongs to "JEP
286: Local-Variable Type Inference" which doesn't seem to be complete
until now. So from my understanding of Mark's "Accelerating the JDK
release cadence" proposal [2], where he writes that "..the bar to
target a JEP to a specific release will, however, be higher since the
work must be Feature Complete in order to go in..", this requires "at
the very least, all necessary code, specification text, and unit
tests" [1].

While JEP 286 has already been targeted for JDK 10, I can only find a
specification attached to the JEP issue but no full implementation and
unit tests. Maybe the implementation is already in the Amber
repository (I can't really judge) but I saw that 'lvti' branch in
Amber was created just two weeks ago.

So to cut a long story short: according to the new development model,
shouldn't the complete JEP 286 be developed and tested in its own repo
or branch and only integrated into jdk10 once it is "feature complete"
?

Thanks,
Volker

[1] http://mail.openjdk.java.net/pipermail/jdk-dev/2017-October/000001.html
[2] http://mail.openjdk.java.net/pipermail/discuss/2017-September/004281.html

On Wed, Oct 11, 2017 at 5:57 PM, Joseph D. Darcy <[hidden email]> wrote:

> Hi Jon,
>
> Catching up post-JavaOne...
>
> On 9/29/2017 11:12 AM, Jonathan Gibbons wrote:
>
> Joe,
>
> I guess I would ask, who are these methods targeted at?
>
> Both isIdentifier and isKeyword are fuzzy at best.
>
> isIdentifier does not take the appropriate Unicode standard into account,
> although it is probably "good enough".
>
>
> Right; specifically it doesn't use the Unicode standard that was in force at
> the time of the earlier releases. Each major JDK platform release generally
> bumps the supported Unicode version and set of identifier characters
> expands. To be 100% accurate the isIdentifier check on release (N - k) for
> some k > 0 would need the Unicode tables used on JDK (N - k). That
> information is not available via the platform APIs.
>
> isKeyword has certainly become more complex in 9 and after, and anyone who
> really needs this info (e.g. IDEs) is probably already doing it for
> themselves, properly.
>
> Do you have any reasonable use-cases for a fuzzy definition of isKeyword, or
> should we just deprecate it words the Big Bit Bucket in the sky?
>
>
> My thought was for annotation processor authors to have some easy ways to
> verify a name was valid to use on a given source version, etc.
>
> I agree tools like IDEs will need more precise implementations of such
> checks, but I think there is still benefit in providing less formal checking
> too.
>
> -Joe
>
Reply | Threaded
Open this post in threaded view
|

Re: Interim JDK 10 RFR of 8187951: Update javax.lang.model.SourceVersion for "var" name

Maurizio Cimadamore


On 12/10/17 11:33, Volker Simonis wrote:

> Hi Joe,
>
> I just have a quick question regarding the overall development
> approach for jdk 10 (aka jdk 18.3).
>
> As Mark has wrote several times (e.g. [1]) "every feature must be
> complete before it's integrated". Your change clearly belongs to "JEP
> 286: Local-Variable Type Inference" which doesn't seem to be complete
> until now. So from my understanding of Mark's "Accelerating the JDK
> release cadence" proposal [2], where he writes that "..the bar to
> target a JEP to a specific release will, however, be higher since the
> work must be Feature Complete in order to go in..", this requires "at
> the very least, all necessary code, specification text, and unit
> tests" [1].
>
> While JEP 286 has already been targeted for JDK 10, I can only find a
> specification attached to the JEP issue but no full implementation and
> unit tests. Maybe the implementation is already in the Amber
> repository (I can't really judge) but I saw that 'lvti' branch in
> Amber was created just two weeks ago.
Hi Volker,
the lvti prototype has been available for several months now; it has
been first discussed here [1]. Then, when we create project Amber, lvti
was one of the first branch to be created [2]. What you see (2 weeks
ago) is the re-creation of same branch when we moved to a consolidated
amber repo - which we did by moving contents over from old
non-consolidated amber repo to the new one. Both repos are still
available for people to see [3].

Unit tests are available (and have been for quite some time) [4], and
the spec text has also been available for some time [5], although it has
recently been rebased and improved (as part of the CSR process).

Ultimately, I think this feature was "complete" when it was integrated
into JDK 10 [6], by some reasonable definition of complete. For every
feature there's always a small tail of things that have been missed, or
that need to be adjusted/tweaked based on feedback - JEP 286 is no
exception to this.

Cheers
Maurizio

[1] -
http://mail.openjdk.java.net/pipermail/platform-jep-discuss/2016-March/000037.html
[2] -
http://mail.openjdk.java.net/pipermail/amber-dev/2017-March/000019.html
[3] - http://hg.openjdk.java.net/amber/
[4] -
http://hg.openjdk.java.net/amber/amber10-old/langtools/file/940ff165a2c3/test/tools/javac/diags/examples/LocalNonDenotable.java
[5] -
http://mail.openjdk.java.net/pipermail/amber-spec-experts/2017-March/000015.html
[6] -http://hg.openjdk.java.net/jdk10/master/rev/48ec75306997


>
> So to cut a long story short: according to the new development model,
> shouldn't the complete JEP 286 be developed and tested in its own repo
> or branch and only integrated into jdk10 once it is "feature complete"
> ?
>
> Thanks,
> Volker
>
> [1] http://mail.openjdk.java.net/pipermail/jdk-dev/2017-October/000001.html
> [2] http://mail.openjdk.java.net/pipermail/discuss/2017-September/004281.html
>
> On Wed, Oct 11, 2017 at 5:57 PM, Joseph D. Darcy <[hidden email]> wrote:
>> Hi Jon,
>>
>> Catching up post-JavaOne...
>>
>> On 9/29/2017 11:12 AM, Jonathan Gibbons wrote:
>>
>> Joe,
>>
>> I guess I would ask, who are these methods targeted at?
>>
>> Both isIdentifier and isKeyword are fuzzy at best.
>>
>> isIdentifier does not take the appropriate Unicode standard into account,
>> although it is probably "good enough".
>>
>>
>> Right; specifically it doesn't use the Unicode standard that was in force at
>> the time of the earlier releases. Each major JDK platform release generally
>> bumps the supported Unicode version and set of identifier characters
>> expands. To be 100% accurate the isIdentifier check on release (N - k) for
>> some k > 0 would need the Unicode tables used on JDK (N - k). That
>> information is not available via the platform APIs.
>>
>> isKeyword has certainly become more complex in 9 and after, and anyone who
>> really needs this info (e.g. IDEs) is probably already doing it for
>> themselves, properly.
>>
>> Do you have any reasonable use-cases for a fuzzy definition of isKeyword, or
>> should we just deprecate it words the Big Bit Bucket in the sky?
>>
>>
>> My thought was for annotation processor authors to have some easy ways to
>> verify a name was valid to use on a given source version, etc.
>>
>> I agree tools like IDEs will need more precise implementations of such
>> checks, but I think there is still benefit in providing less formal checking
>> too.
>>
>> -Joe
>>

Reply | Threaded
Open this post in threaded view
|

Re: Interim JDK 10 RFR of 8187951: Update javax.lang.model.SourceVersion for "var" name

Volker Simonis
Hi Maurizio,

thanks a lot for the detailed information. The integration indeed
makes more sense now.

I just asked because we had our first JCP JDK 18.3 EG meeting today
and I try to understand how the JEP process and the JSR JCP process
work together (JEP 286 is currently the only JEP targeted for Java
18.3 so I had to beat on you :).

In general (and this is not related to your JEP anymore which seems in
good shape) I think it would be nice if every JEP which is proposed to
be targeted contains some information about where the implementation /
test / specification can be found (binaries to easily test the new
feature would always be a plus of corse :)

Thanks,
Volker


On Thu, Oct 12, 2017 at 6:57 AM, Maurizio Cimadamore
<[hidden email]> wrote:

>
>
> On 12/10/17 11:33, Volker Simonis wrote:
>>
>> Hi Joe,
>>
>> I just have a quick question regarding the overall development
>> approach for jdk 10 (aka jdk 18.3).
>>
>> As Mark has wrote several times (e.g. [1]) "every feature must be
>> complete before it's integrated". Your change clearly belongs to "JEP
>> 286: Local-Variable Type Inference" which doesn't seem to be complete
>> until now. So from my understanding of Mark's "Accelerating the JDK
>> release cadence" proposal [2], where he writes that "..the bar to
>> target a JEP to a specific release will, however, be higher since the
>> work must be Feature Complete in order to go in..", this requires "at
>> the very least, all necessary code, specification text, and unit
>> tests" [1].
>>
>> While JEP 286 has already been targeted for JDK 10, I can only find a
>> specification attached to the JEP issue but no full implementation and
>> unit tests. Maybe the implementation is already in the Amber
>> repository (I can't really judge) but I saw that 'lvti' branch in
>> Amber was created just two weeks ago.
>
> Hi Volker,
> the lvti prototype has been available for several months now; it has been
> first discussed here [1]. Then, when we create project Amber, lvti was one
> of the first branch to be created [2]. What you see (2 weeks ago) is the
> re-creation of same branch when we moved to a consolidated amber repo -
> which we did by moving contents over from old non-consolidated amber repo to
> the new one. Both repos are still available for people to see [3].
>
> Unit tests are available (and have been for quite some time) [4], and the
> spec text has also been available for some time [5], although it has
> recently been rebased and improved (as part of the CSR process).
>
> Ultimately, I think this feature was "complete" when it was integrated into
> JDK 10 [6], by some reasonable definition of complete. For every feature
> there's always a small tail of things that have been missed, or that need to
> be adjusted/tweaked based on feedback - JEP 286 is no exception to this.
>
> Cheers
> Maurizio
>
> [1] -
> http://mail.openjdk.java.net/pipermail/platform-jep-discuss/2016-March/000037.html
> [2] -
> http://mail.openjdk.java.net/pipermail/amber-dev/2017-March/000019.html
> [3] - http://hg.openjdk.java.net/amber/
> [4] -
> http://hg.openjdk.java.net/amber/amber10-old/langtools/file/940ff165a2c3/test/tools/javac/diags/examples/LocalNonDenotable.java
> [5] -
> http://mail.openjdk.java.net/pipermail/amber-spec-experts/2017-March/000015.html
> [6] -http://hg.openjdk.java.net/jdk10/master/rev/48ec75306997
>
>
>
>>
>> So to cut a long story short: according to the new development model,
>> shouldn't the complete JEP 286 be developed and tested in its own repo
>> or branch and only integrated into jdk10 once it is "feature complete"
>> ?
>>
>> Thanks,
>> Volker
>>
>> [1]
>> http://mail.openjdk.java.net/pipermail/jdk-dev/2017-October/000001.html
>> [2]
>> http://mail.openjdk.java.net/pipermail/discuss/2017-September/004281.html
>>
>> On Wed, Oct 11, 2017 at 5:57 PM, Joseph D. Darcy <[hidden email]>
>> wrote:
>>>
>>> Hi Jon,
>>>
>>> Catching up post-JavaOne...
>>>
>>> On 9/29/2017 11:12 AM, Jonathan Gibbons wrote:
>>>
>>> Joe,
>>>
>>> I guess I would ask, who are these methods targeted at?
>>>
>>> Both isIdentifier and isKeyword are fuzzy at best.
>>>
>>> isIdentifier does not take the appropriate Unicode standard into account,
>>> although it is probably "good enough".
>>>
>>>
>>> Right; specifically it doesn't use the Unicode standard that was in force
>>> at
>>> the time of the earlier releases. Each major JDK platform release
>>> generally
>>> bumps the supported Unicode version and set of identifier characters
>>> expands. To be 100% accurate the isIdentifier check on release (N - k)
>>> for
>>> some k > 0 would need the Unicode tables used on JDK (N - k). That
>>> information is not available via the platform APIs.
>>>
>>> isKeyword has certainly become more complex in 9 and after, and anyone
>>> who
>>> really needs this info (e.g. IDEs) is probably already doing it for
>>> themselves, properly.
>>>
>>> Do you have any reasonable use-cases for a fuzzy definition of isKeyword,
>>> or
>>> should we just deprecate it words the Big Bit Bucket in the sky?
>>>
>>>
>>> My thought was for annotation processor authors to have some easy ways to
>>> verify a name was valid to use on a given source version, etc.
>>>
>>> I agree tools like IDEs will need more precise implementations of such
>>> checks, but I think there is still benefit in providing less formal
>>> checking
>>> too.
>>>
>>> -Joe
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re Take 2 of JDK 10 RFR of 8187951: Update javax.lang.model.SourceVersion for "var" name

joe darcy
In reply to this post by joe darcy
Hello,

A second iteration for your consideration:

     http://cr.openjdk.java.net/~darcy/8187951.1/

In a specification sense, this iteration of the proposed change just
documents the existing behavior on the string "var" and tests are added
to verify that behavior. The handling of "var" is analogous to the
handling of restricted keywords. (The webrev is generated against JDK
11, but I'll rebase to JDK 10 before pushing.)

Corresponding CSR for review at:

     https://bugs.openjdk.java.net/browse/JDK-8194523

Thanks,

-Joe


Reply | Threaded
Open this post in threaded view
|

Re: Re Take 2 of JDK 10 RFR of 8187951: Update javax.lang.model.SourceVersion for "var" name

Maurizio Cimadamore
Looks good

Maurizio


On 03/01/18 18:32, joe darcy wrote:

> Hello,
>
> A second iteration for your consideration:
>
>     http://cr.openjdk.java.net/~darcy/8187951.1/
>
> In a specification sense, this iteration of the proposed change just
> documents the existing behavior on the string "var" and tests are
> added to verify that behavior. The handling of "var" is analogous to
> the handling of restricted keywords. (The webrev is generated against
> JDK 11, but I'll rebase to JDK 10 before pushing.)
>
> Corresponding CSR for review at:
>
>     https://bugs.openjdk.java.net/browse/JDK-8194523
>
> Thanks,
>
> -Joe
>
>