New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JavadocTokenTypes should keep values of tokens, restore tokens as they were at 8.1 version #5139

Closed
romani opened this Issue Sep 23, 2017 · 14 comments

Comments

Projects
None yet
4 participants
@romani
Member

romani commented Sep 23, 2017

create GeneratedJavadocTokenTypesTest.
see java analog - https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/grammars/GeneratedJavaTokenTypesTest.java

it is required to avoid serious compatibility problems, as static values are compiled as inlined values in other projects, changes in numbers will cause serious damage to Checks behavior.

See example of compatibility problems at #505 .

checkstyle/target/generated-sources/antlr/com/puppycrawl/tools/checkstyle/grammars/javadoc/JavadocParser.java

There is no compatibility damage between 8.0 and 8.1.

compatibility damage between 8.1 and 8.2:
image

compatibility damage between 8.2 and 8.3-SNAPSHOT:
image

We need to restore token values to 8.1 state finally.
as first step we can enforce values that we have in 8.3-SNAPSHOT now.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 23, 2017

Member

@ps-sp , please help us to address this ASAP, we are at serious compatibility problem.
Please send PR with enforcing 8.3-SPANSHOT values, not allow further regression from PRs.
We need to restore values as it was before, change in names is not a problem.

@Vladlis , FYI.

Member

romani commented Sep 23, 2017

@ps-sp , please help us to address this ASAP, we are at serious compatibility problem.
Please send PR with enforcing 8.3-SPANSHOT values, not allow further regression from PRs.
We need to restore values as it was before, change in names is not a problem.

@Vladlis , FYI.

@romani romani changed the title from create GeneratedJavadocTokenTypesTest to JavadocTokenTypes should keep values of tokens Sep 23, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach
Member

rnveach commented Sep 24, 2017

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Sep 25, 2017

Collaborator

@romani
Sorry for the delay. I will try to get things done as soon as possible, bit occupied at the moment, sorry.

Collaborator

ps-sp commented Sep 25, 2017

@romani
Sorry for the delay. I will try to get things done as soon as possible, bit occupied at the moment, sorry.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 9, 2017

Member

issue for UT creation is created - #5186

Member

romani commented Oct 9, 2017

issue for UT creation is created - #5186

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 20, 2017

Member

@ps-sp ,
thanks a lot for fix at #5186

and now .... can we restore to previous values , as they were at 8.1 version ?

Member

romani commented Oct 20, 2017

@ps-sp ,
thanks a lot for fix at #5186

and now .... can we restore to previous values , as they were at 8.1 version ?

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Oct 21, 2017

Collaborator

@romani @Vladlis @rnveach

Right now I can't see any graceful way to override the token and rule numbers generated by ANTLR since apparently there's no mechanism provided by ANTLR to naturally override the order of generation of token or rule numbers.

ANTLR seems to generate token or rule numbers in the order in which it encounters the corresponding elements in the grammar. So new tokens and new rules can be added at the end of the grammar to preserve the previous numberings but if one wishes to append to an existing rule, like in #3332 for example, then all the rule numbers beyond that rule shall change. Same goes if one has to delete a token, though this can may be worked around by removing all the token uses from parser grammar and letting its definition stay in the lexer grammar. Anyway, the point is that there can be some certain kind of updates to grammar in which we may not be able to naturally preserve token and rule numbers.

That said, one way I can think of to overcome this is by completely decoupling JavadocTokenTypes from the parser generated token and rule numbers. We can hard code the constants in JavadocTokenTypes and map the corresponding token and rule numbers in the generated class JavadocParser to their corresponding values in JavadocTokenTypes. We can do this mapping either via reflection or explicitly. This way we will be able to have complete control over the token numbers we provide to the users. We will need to update the getType and setType methods in DetailNode implementation accordingly.

What do you guys think ?

Collaborator

ps-sp commented Oct 21, 2017

@romani @Vladlis @rnveach

Right now I can't see any graceful way to override the token and rule numbers generated by ANTLR since apparently there's no mechanism provided by ANTLR to naturally override the order of generation of token or rule numbers.

ANTLR seems to generate token or rule numbers in the order in which it encounters the corresponding elements in the grammar. So new tokens and new rules can be added at the end of the grammar to preserve the previous numberings but if one wishes to append to an existing rule, like in #3332 for example, then all the rule numbers beyond that rule shall change. Same goes if one has to delete a token, though this can may be worked around by removing all the token uses from parser grammar and letting its definition stay in the lexer grammar. Anyway, the point is that there can be some certain kind of updates to grammar in which we may not be able to naturally preserve token and rule numbers.

That said, one way I can think of to overcome this is by completely decoupling JavadocTokenTypes from the parser generated token and rule numbers. We can hard code the constants in JavadocTokenTypes and map the corresponding token and rule numbers in the generated class JavadocParser to their corresponding values in JavadocTokenTypes. We can do this mapping either via reflection or explicitly. This way we will be able to have complete control over the token numbers we provide to the users. We will need to update the getType and setType methods in DetailNode implementation accordingly.

What do you guys think ?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 22, 2017

Member

So new tokens and new rules can be added at the end of the grammar to preserve the previous numberings but if one wishes to append to an existing rule, like in #3332 for example, then all the rule numbers beyond that rule shall change.

please give a link to update where new grammar elements has to be added in the middle of grammar, not to the end of grammar.
Decoupling is last resort approach only.

Member

romani commented Oct 22, 2017

So new tokens and new rules can be added at the end of the grammar to preserve the previous numberings but if one wishes to append to an existing rule, like in #3332 for example, then all the rule numbers beyond that rule shall change.

please give a link to update where new grammar elements has to be added in the middle of grammar, not to the end of grammar.
Decoupling is last resort approach only.

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Oct 22, 2017

Collaborator

@romani

Ohh wait. For example in #4943 when embed tag was added as an alternative to an existing rule, I (wrongly) thought that it might change the rule numbers but that might not be the case. Let me have a look again and get back to you.

Collaborator

ps-sp commented Oct 22, 2017

@romani

Ohh wait. For example in #4943 when embed tag was added as an alternative to an existing rule, I (wrongly) thought that it might change the rule numbers but that might not be the case. Let me have a look again and get back to you.

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Oct 23, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Oct 24, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Oct 24, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Oct 24, 2017

rnveach added a commit that referenced this issue Oct 26, 2017

rnveach added a commit that referenced this issue Oct 26, 2017

rnveach added a commit that referenced this issue Oct 26, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 26, 2017

Member

Fix was merged

Member

rnveach commented Oct 26, 2017

Fix was merged

@rnveach rnveach closed this Oct 26, 2017

@romani romani added this to the 8.4 milestone Oct 27, 2017

@romani romani changed the title from JavadocTokenTypes should keep values of tokens to JavadocTokenTypes should keep values of tokens, restore tokens as they were at 8.1 version Oct 27, 2017

@Vladlis

This comment has been minimized.

Show comment
Hide comment
@Vladlis

Vladlis Oct 27, 2017

Member

@ps-sp Thanks!

Member

Vladlis commented Oct 27, 2017

@ps-sp Thanks!

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jan 10, 2018

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jan 14, 2018

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jan 16, 2018

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jan 18, 2018

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jan 23, 2018

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 11, 2018

Member

We have two options:

  1. leave breaking compatibility tat we already caused. And force all thirparty javadoc Checks to recompile. But be careful in future with grammar updates and make decision later on, when we come to case that will force us to decouple Tokens.

2)Looks like decoupling TokenTypes from ParserTokens is only way out. We should use map function after parsing to convert tree to TokenTypes from ParserTokens.

I do not see another solutions.

@ps-sp , @rnveach , @Vladlis , please share your thoughts .... what approach it better for you?

Approach "1)" is not an option .... as we already come to cases where addition of new token result in breaking compatibility, see PR #5101 (Issue #3332) .
Looks like 2) is the only way out.

Member

romani commented Mar 11, 2018

We have two options:

  1. leave breaking compatibility tat we already caused. And force all thirparty javadoc Checks to recompile. But be careful in future with grammar updates and make decision later on, when we come to case that will force us to decouple Tokens.

2)Looks like decoupling TokenTypes from ParserTokens is only way out. We should use map function after parsing to convert tree to TokenTypes from ParserTokens.

I do not see another solutions.

@ps-sp , @rnveach , @Vladlis , please share your thoughts .... what approach it better for you?

Approach "1)" is not an option .... as we already come to cases where addition of new token result in breaking compatibility, see PR #5101 (Issue #3332) .
Looks like 2) is the only way out.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 12, 2018

Member

Looks like decoupling TokenTypes from ParserTokens is only way out. We should use map function after parsing to convert tree to TokenTypes from ParserTokens.
Looks like 2) is the only way out.

We have already started this in #5455 with the method getJavadocTokenType.

Member

rnveach commented Mar 12, 2018

Looks like decoupling TokenTypes from ParserTokens is only way out. We should use map function after parsing to convert tree to TokenTypes from ParserTokens.
Looks like 2) is the only way out.

We have already started this in #5455 with the method getJavadocTokenType.

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 12, 2018

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 12, 2018

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 2, 2018

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 2, 2018

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 3, 2018

Member

this issue will be dropped for #5880

Member

rnveach commented Jun 3, 2018

this issue will be dropped for #5880

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 3, 2018

Member

This issue was closed as we introduced one more damage to JavadocTokenTypes at PR #5101 (Issue #3332). No sense to restore to 8.1 tokens.
Unfortunately all affected extensions should be recompiled.

Member

romani commented Jun 3, 2018

This issue was closed as we introduced one more damage to JavadocTokenTypes at PR #5101 (Issue #3332). No sense to restore to 8.1 tokens.
Unfortunately all affected extensions should be recompiled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment