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

Mark all checks with appropriate annotation so that they could be used in the MT mode #4870

Closed
soon opened this Issue Jul 31, 2017 · 13 comments

Comments

4 participants
@soon
Contributor

soon commented Jul 31, 2017

Subtask of #4473

Each check must implement either OneCheckInstancePerApplication or OneCheckInstancePerThread.

All changes for this task must be just implements OneCheckInstancePerApplication or implements OneCheckInstancePerThread, i.e. no more logic is expected. If a particular check requires additional changes, there should be a separate issue for that check.


Please expand in issue what are the requirements for a module to be marked with a specific interface.

1, If a check holds file state (i.e. non-final fields, non-property fields) it must be marked with OneCheckInstancePerThread interface.
2. If a check is stateless (all fields are final or properties, and there are no mutable fields like sets or lists) it must be marked with OneCheckInstancePerApplication. Note, that a "mutable field" here - is the field which may change it's state while the Check processes a file.
3. If a check is global-stateful (it contains a mutable state, which must be updated across the whole application lifetime), it must be marked with OneCheckInstancePerApplication and all state updates must be thread-safe.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 7, 2017

Member

If a check is a global-stateful

Why don't we have a unique interface for this?
Even if it acts the same as OneCheckInstancePerApplication won't there be some benefit from separating them out even if it is just for users/us to know?

If a check holds file state (i.e. non-final fields, non-property fields) it must be marked with OneCheckInstancePerThread interface.

By this definition alone, doesn't it mean AbstractCheck holds state as it has a few non-final fields. Whatever we define AbstractCheck as, it will extend to all classes that extend it too.
Or do we only examine change in states after it has been initialized/cloned?

Member

rnveach commented Aug 7, 2017

If a check is a global-stateful

Why don't we have a unique interface for this?
Even if it acts the same as OneCheckInstancePerApplication won't there be some benefit from separating them out even if it is just for users/us to know?

If a check holds file state (i.e. non-final fields, non-property fields) it must be marked with OneCheckInstancePerThread interface.

By this definition alone, doesn't it mean AbstractCheck holds state as it has a few non-final fields. Whatever we define AbstractCheck as, it will extend to all classes that extend it too.
Or do we only examine change in states after it has been initialized/cloned?

@soon

This comment has been minimized.

Show comment
Hide comment
@soon

soon Aug 7, 2017

Contributor

Why don't we have a unique interface for this?
Even if it acts the same as OneCheckInstancePerApplication won't there be some benefit from separating them out even if it is just for users/us to know?

@rnveach We can, sure, but I don't see a reason for it. Maybe in the future it will be useful. Should I add it?

By this definition alone, doesn't it mean AbstractCheck holds state as it has a few non-final fields. Whatever we define AbstractCheck as, it will extend to all classes that extend it too.
Or do we only examine change in states after it has been initialized/cloned?

The AbstractCheck is global stateful now, yes. It will require some changes, so that messages could be used from multiple threads.

Contributor

soon commented Aug 7, 2017

Why don't we have a unique interface for this?
Even if it acts the same as OneCheckInstancePerApplication won't there be some benefit from separating them out even if it is just for users/us to know?

@rnveach We can, sure, but I don't see a reason for it. Maybe in the future it will be useful. Should I add it?

By this definition alone, doesn't it mean AbstractCheck holds state as it has a few non-final fields. Whatever we define AbstractCheck as, it will extend to all classes that extend it too.
Or do we only examine change in states after it has been initialized/cloned?

The AbstractCheck is global stateful now, yes. It will require some changes, so that messages could be used from multiple threads.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 7, 2017

Member

Should I add it?

I think we should.
@romani What do you think?

It will require some changes, so that messages could be used from multiple threads.

Doesn't this have to be done first before we even implement the MT modes?

The AbstractCheck is global stateful now, yes

If messages is the problem, isn't it more file stateful then as messages are based on a file and not the entire application as a whole?
Also if messages is the problem, it is declared final and is stateless based on your written requirements.

Member

rnveach commented Aug 7, 2017

Should I add it?

I think we should.
@romani What do you think?

It will require some changes, so that messages could be used from multiple threads.

Doesn't this have to be done first before we even implement the MT modes?

The AbstractCheck is global stateful now, yes

If messages is the problem, isn't it more file stateful then as messages are based on a file and not the entire application as a whole?
Also if messages is the problem, it is declared final and is stateless based on your written requirements.

@soon

This comment has been minimized.

Show comment
Hide comment
@soon

soon Aug 7, 2017

Contributor

Also if messages is the problem, it is declared final and is stateless based on your written requirements.

I've updated the post

Doesn't this have to be done first before we even implement the MT modes?

Created issue #4908, preparing PR

If messages is the problem, isn't it more file stateful then as messages are based on a file and not the entire application as a whole?

Not sure if I understood you properly. We cannot make the AbstractCheck file-stateful, because all subclassess will be file-statefull too, no mater if they contain a state or not.

Contributor

soon commented Aug 7, 2017

Also if messages is the problem, it is declared final and is stateless based on your written requirements.

I've updated the post

Doesn't this have to be done first before we even implement the MT modes?

Created issue #4908, preparing PR

If messages is the problem, isn't it more file stateful then as messages are based on a file and not the entire application as a whole?

Not sure if I understood you properly. We cannot make the AbstractCheck file-stateful, because all subclassess will be file-statefull too, no mater if they contain a state or not.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 7, 2017

Member

We cannot make the AbstractCheck file-stateful, because all subclassess will be file-statefull too, no mater if they contain a state or not.

That is correct, and what I was talking about in one of the PRs where you were marking some abstract classes with these interfaces.

My statement was how can AbstractCheck be completely stateless if it holds messages of a file? That alone makes it sound like it will never be anything but file-stateful until messages it completely removed that class.
I haven't looked at your new PR yet.

Member

rnveach commented Aug 7, 2017

We cannot make the AbstractCheck file-stateful, because all subclassess will be file-statefull too, no mater if they contain a state or not.

That is correct, and what I was talking about in one of the PRs where you were marking some abstract classes with these interfaces.

My statement was how can AbstractCheck be completely stateless if it holds messages of a file? That alone makes it sound like it will never be anything but file-stateful until messages it completely removed that class.
I haven't looked at your new PR yet.

soon added a commit to soon/checkstyle that referenced this issue Aug 9, 2017

soon added a commit to soon/checkstyle that referenced this issue Aug 9, 2017

soon added a commit to soon/checkstyle that referenced this issue Aug 9, 2017

soon added a commit to soon/checkstyle that referenced this issue Aug 9, 2017

soon added a commit to soon/checkstyle that referenced this issue Aug 9, 2017

@soon

This comment has been minimized.

Show comment
Hide comment
@soon

soon Aug 14, 2017

Contributor

I want to discuss the IllegalTypeCheck. I marked it as global-stateful check, because it updates field illegalClassNames across the application lifetime. In this case we have to make this field thread-safe.
However, seems like the check could be file-stateful as well

@rnveach @sabaka @romani what do you think?

Contributor

soon commented Aug 14, 2017

I want to discuss the IllegalTypeCheck. I marked it as global-stateful check, because it updates field illegalClassNames across the application lifetime. In this case we have to make this field thread-safe.
However, seems like the check could be file-stateful as well

@rnveach @sabaka @romani what do you think?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 14, 2017

Member

seems like the check could be file-stateful as well

It should be file-stateful as it has to look at imports for a class, imo.

it updates field illegalClassNames across the application lifetime.

I assume you are referring to this line:

as it is called in visitImport.
This does seem like weird behavior. What is even more weird, if I remove all the code in visitImport no tests fail. Mutation is a 95%, so this could be the class that is holding it back. I would need to run regression with it gone to see if there is some behavior we are missing.

Feel free to create a new issue on this.

Edit:
Default module showed no differences.
http://rveach.no-ip.org/checkstyle/regression/reports/101/
Differences only show up when illegalClassNames is populated by the user.
http://rveach.no-ip.org/checkstyle/regression/reports/102/

Member

rnveach commented Aug 14, 2017

seems like the check could be file-stateful as well

It should be file-stateful as it has to look at imports for a class, imo.

it updates field illegalClassNames across the application lifetime.

I assume you are referring to this line:

as it is called in visitImport.
This does seem like weird behavior. What is even more weird, if I remove all the code in visitImport no tests fail. Mutation is a 95%, so this could be the class that is holding it back. I would need to run regression with it gone to see if there is some behavior we are missing.

Feel free to create a new issue on this.

Edit:
Default module showed no differences.
http://rveach.no-ip.org/checkstyle/regression/reports/101/
Differences only show up when illegalClassNames is populated by the user.
http://rveach.no-ip.org/checkstyle/regression/reports/102/

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 14, 2017

Member

@soon I am moving issue with IllegalTypeCheck to a new issue.

Member

rnveach commented Aug 14, 2017

@soon I am moving issue with IllegalTypeCheck to a new issue.

soon added a commit to soon/checkstyle that referenced this issue Oct 11, 2017

soon added a commit to soon/checkstyle that referenced this issue Oct 11, 2017

soon added a commit to soon/checkstyle that referenced this issue Oct 15, 2017

romani added a commit that referenced this issue Oct 31, 2017

@romani romani added this to the 8.5 milestone Oct 31, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 31, 2017

Member

@soon,

How it is planned to treat Checks without annotations ? there thousands of custom Checks in the world, we should continue to work with such Checks.
What changes you expect for example for all sevntu Checks ?

Member

romani commented Oct 31, 2017

@soon,

How it is planned to treat Checks without annotations ? there thousands of custom Checks in the world, we should continue to work with such Checks.
What changes you expect for example for all sevntu Checks ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 31, 2017

Member

@romani I made sure un-annotated checks are treated no differently then they are today. (Edit: think this was the original post #4882 (comment) )
See also my comment at #5140 (comment) re-iterating this. We placed it into documentation at

* Note: Checks with such annotation will be executed in mode how all Checks worked
* before MT mode is introduced.

Member

rnveach commented Oct 31, 2017

@romani I made sure un-annotated checks are treated no differently then they are today. (Edit: think this was the original post #4882 (comment) )
See also my comment at #5140 (comment) re-iterating this. We placed it into documentation at

* Note: Checks with such annotation will be executed in mode how all Checks worked
* before MT mode is introduced.

soon added a commit to soon/checkstyle that referenced this issue Nov 14, 2017

soon added a commit to soon/checkstyle that referenced this issue Dec 17, 2017

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

soon added a commit to soon/checkstyle that referenced this issue Feb 11, 2018

@romani romani removed the GSoC2017 label Jun 3, 2018

rnveach added a commit to soon/checkstyle that referenced this issue Jun 8, 2018

@romani romani changed the title from Mark all checks with appropriate interface so that they could be used in the MT mode to Mark all checks with appropriate annotation so that they could be used in the MT mode Jul 7, 2018

romani added a commit to romani/checkstyle that referenced this issue Jul 7, 2018

@romani romani unassigned soon Jul 7, 2018

rnveach added a commit that referenced this issue Jul 8, 2018

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 8, 2018

Member

I am not closing this issue yet.
I think we should add a UT to show all checks are marked. We added some checks since this issue was originally opened.

Member

rnveach commented Jul 8, 2018

I am not closing this issue yet.
I think we should add a UT to show all checks are marked. We added some checks since this issue was originally opened.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 8, 2018

Member

I will work on the test.

The following modules are missing an annotation:

JavadocTagContinuationIndentationCheck
IllegalTypeCheck
RegexpSinglelineCheck
AnnotationOnSameLineCheck
TreeWalker
NonEmptyAtclauseDescriptionCheck
SingleLineJavadocCheck
AtclauseOrderCheck
SummaryJavadocCheck
JavadocParagraphCheck
RegexpMultilineCheck

Member

rnveach commented Jul 8, 2018

I will work on the test.

The following modules are missing an annotation:

JavadocTagContinuationIndentationCheck
IllegalTypeCheck
RegexpSinglelineCheck
AnnotationOnSameLineCheck
TreeWalker
NonEmptyAtclauseDescriptionCheck
SingleLineJavadocCheck
AtclauseOrderCheck
SummaryJavadocCheck
JavadocParagraphCheck
RegexpMultilineCheck

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 8, 2018

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 8, 2018

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 8, 2018

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 8, 2018

rnveach added a commit to rnveach/checkstyle that referenced this issue Jul 8, 2018

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 9, 2018

Member

Fix is merged

Member

romani commented Jul 9, 2018

Fix is merged

@romani romani closed this Jul 9, 2018

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