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

Checker Cache not saving files that have suppressed violations #3488

Closed
rnveach opened this Issue Oct 2, 2016 · 8 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Oct 2, 2016

After testing out a fix for #3487 and examining the cache file, I noticed only the main directory was being stored in the cache and nothing from test or it. After checking out the code, it seems checkstyle does not cache those files because they have violations.

if (cache != null && fileMessages.isEmpty()) {

Checkstyle has no violations being printed during its run. All the violations that are preventing files from being cached are all suppressed inside fireErrors.
Since suppressions are part of the external resources, we can add these files to the cache without having to worry about a disconnect between cache and non-cached runs. When the external resource is changed, the entire cache file is invalidated.
We should only be skipping placing files in the cache if they print non-suppressedd violations.

The issue can be seen if you run the ant-phase-verify multiple times without changing anything and having the cache on. The second run should finish instantly since no files have changed and there are no violations.

this issue depends on #3489.


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 3, 2016

Member

I think we should either remove errors from the set in Checker.fireErrors when they are suppressed, or use a field to pass information out of the method to verify if any violations were not suppressed. Any other type of change, would involve changing fireErrors API.

Edit: Since we can call fireErrors in AbstractFileSetCheck during finishProcessing (TranslationCheck), we have to add all files to the cache, and then remove them if errors get fired that are not suppressed.

Member

rnveach commented Oct 3, 2016

I think we should either remove errors from the set in Checker.fireErrors when they are suppressed, or use a field to pass information out of the method to verify if any violations were not suppressed. Any other type of change, would involve changing fireErrors API.

Edit: Since we can call fireErrors in AbstractFileSetCheck during finishProcessing (TranslationCheck), we have to add all files to the cache, and then remove them if errors get fired that are not suppressed.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 13, 2016

Member

I noticed only the main directory was being stored in the cache and nothing from test or it

we have suppressions over main code too.

We should only be skipping placing files in the cache if they print non-suppressedd violations.

this is true only after
we implemented caching of external resource/config files - already done.
and when config itself is in cache - look like not always true due to another bug in caching.

Member

romani commented Oct 13, 2016

I noticed only the main directory was being stored in the cache and nothing from test or it

we have suppressions over main code too.

We should only be skipping placing files in the cache if they print non-suppressedd violations.

this is true only after
we implemented caching of external resource/config files - already done.
and when config itself is in cache - look like not always true due to another bug in caching.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 13, 2016

Member

we have suppressions over main code too.

But not in every single file in main. The bug only happens when a violation is suppressed on any given run. Not all main code has suppressed violations. When I said only the main directory was being stored I meant this as a general reference. Doesn't mean every file in main was listed, just a lot were.

The main point is we suppress all violations in Checkstyle to give us no errors, so every file should be listed in the cache.

Member

rnveach commented Oct 13, 2016

we have suppressions over main code too.

But not in every single file in main. The bug only happens when a violation is suppressed on any given run. Not all main code has suppressed violations. When I said only the main directory was being stored I meant this as a general reference. Doesn't mean every file in main was listed, just a lot were.

The main point is we suppress all violations in Checkstyle to give us no errors, so every file should be listed in the cache.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 13, 2016

Member

The main point is we suppress all violations in Checkstyle to give us no errors, so every file should be listed in the cache.

in our case, our project, yes.

Member

romani commented Oct 13, 2016

The main point is we suppress all violations in Checkstyle to give us no errors, so every file should be listed in the cache.

in our case, our project, yes.

@romani romani added the approved label Oct 13, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 13, 2016

Member

@rnveach

Edit: Since we can call fireErrors in AbstractFileSetCheck during finishProcessing (TranslationCheck), we have to add all files to the cache, and then remove them if errors get fired that are not suppressed.

In the code of Checker there is problem of wrong reusage of fireErrors mehod.
All violations are received from final SortedSet<LocalizedMessage> fileMessages = processFile(file);
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Checker.java#L266
fireErrors execution is wrong as it is how Exceptions from Checks should be printed. fireErrors should be continued to be used by abstract Checks to deliver ERROR to Checker. Check log/report violations , Check do not know severity level. For Check, ERROR is exceptional state that need to be reported.

Current implementation of Checker.fireError need to splt between fireViolations(with suppressions) and fireErrors(that is only for exceptional Errors).

fireViolation should receive list of messages that need to be printed. All filtering should be done out of fireViolation methods. This will let us make a decision to place file to cache or not.

Member

romani commented Oct 13, 2016

@rnveach

Edit: Since we can call fireErrors in AbstractFileSetCheck during finishProcessing (TranslationCheck), we have to add all files to the cache, and then remove them if errors get fired that are not suppressed.

In the code of Checker there is problem of wrong reusage of fireErrors mehod.
All violations are received from final SortedSet<LocalizedMessage> fileMessages = processFile(file);
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Checker.java#L266
fireErrors execution is wrong as it is how Exceptions from Checks should be printed. fireErrors should be continued to be used by abstract Checks to deliver ERROR to Checker. Check log/report violations , Check do not know severity level. For Check, ERROR is exceptional state that need to be reported.

Current implementation of Checker.fireError need to splt between fireViolations(with suppressions) and fireErrors(that is only for exceptional Errors).

fireViolation should receive list of messages that need to be printed. All filtering should be done out of fireViolation methods. This will let us make a decision to place file to cache or not.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 13, 2016

Member

I moved your idea to splitting fireError into a new issue - #3503.

Below are some of our comments from chat:

All violations are received from final SortedSet<LocalizedMessage> fileMessages = processFile(file);

All is wrong because some checks, most notably, TranslationCheck, fire errors in finishProcessing, which is further after processFile. This is because the checks need to look at all files before making a decision on final violations, which it can't decide looking at one file at a time.
Outside of Checkstyle, I also have some custom checks that too must examine all files before violations can be decided.

All filtering should be done out of fireViolation methods. This will let us make a decision to place file to cache or not.

The decision to place or not still can't be inside fireViolation alone. Since we fire violations in 2 different areas, we can't make a final determination if the file should be in the cache or not, until all checks are done. This is why my idea and PR, place all files into the cache and then remove them as violations are printed.

If we move printing violations to a single spot after finishProcessing to overcome this issue, then we run into memory and possibly performance issues because now we have to keep all violations in memory and re-loop through the files to figure out which files gave no violation. Currently we only keep 1 file's worth of violations in memory and print them as soon as the file is done.

Member

rnveach commented Oct 13, 2016

I moved your idea to splitting fireError into a new issue - #3503.

Below are some of our comments from chat:

All violations are received from final SortedSet<LocalizedMessage> fileMessages = processFile(file);

All is wrong because some checks, most notably, TranslationCheck, fire errors in finishProcessing, which is further after processFile. This is because the checks need to look at all files before making a decision on final violations, which it can't decide looking at one file at a time.
Outside of Checkstyle, I also have some custom checks that too must examine all files before violations can be decided.

All filtering should be done out of fireViolation methods. This will let us make a decision to place file to cache or not.

The decision to place or not still can't be inside fireViolation alone. Since we fire violations in 2 different areas, we can't make a final determination if the file should be in the cache or not, until all checks are done. This is why my idea and PR, place all files into the cache and then remove them as violations are printed.

If we move printing violations to a single spot after finishProcessing to overcome this issue, then we run into memory and possibly performance issues because now we have to keep all violations in memory and re-loop through the files to figure out which files gave no violation. Currently we only keep 1 file's worth of violations in memory and print them as soon as the file is done.

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 14, 2016

@romani romani changed the title from Checker Cache not saving files with suppressions to Checker Cache not saving files that have suppressed violations Nov 10, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 10, 2016

Member

all references of TranslationCheck should not be considered at all during this issue.
TranslationCheck is unfortunate mis-design inside of our library, it is multi-file validation tool, but Checkstyle is single-file validation tool. There bunch of nuances with cache and problems in design how multi-file modules report violations.
Multi-file validation will be considered in another issue.

Attention:
In this issue we will resolve only consideration of filtered/suppressed violations when do decision about placing into cache.

Member

romani commented Nov 10, 2016

all references of TranslationCheck should not be considered at all during this issue.
TranslationCheck is unfortunate mis-design inside of our library, it is multi-file validation tool, but Checkstyle is single-file validation tool. There bunch of nuances with cache and problems in design how multi-file modules report violations.
Multi-file validation will be considered in another issue.

Attention:
In this issue we will resolve only consideration of filtered/suppressed violations when do decision about placing into cache.

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 10, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 10, 2016

romani added a commit that referenced this issue Nov 10, 2016

@romani romani added the bug label Nov 10, 2016

@romani romani added this to the 7.3 milestone Nov 10, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 10, 2016

Member

fix is merged, should work for all except for TranslatonCheck.

Member

romani commented Nov 10, 2016

fix is merged, should work for all except for TranslatonCheck.

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