Skip to content
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

msgs: more decision making in handleReport #948

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Oct 7, 2023

Summary

  • fix hints/warnings promoted to errors always counting towards the
    maximum error count, even if the hint/warning is currently disabled
  • fix disabled warnings and hints being reported by nimsuggest chk

Details

Processing of a Report previously worked as follows:

  1. handleReport pre-processes the report
  2. the report is passed to the report hook
  3. the hook outputs the report if it determines that the report should
    be rendered and displayed
  4. handleReport determines the error handling strategy
  5. error handling is performed

This has two problems:

  1. the responsibility of deciding whether a report should be output is
    left to the hook, which also means that the hook needs access to all
    state necessary to make the decision
  2. error handling always takes place, even if the report is disabled

Report handling is split up, and the responsibilities are shifted:

  1. if a report is disabled (isEnabled returns false), handleReport
    is a no-op
  2. deciding writability is now the responsibility of handleReport
    (i.e., the compiler), with the report hook only invoked if the
    report is eligible for being displayed
  3. computing writability no longer takes into account whether the
    report is enabled (since writeability is only decided after it is
    known that a report is enabled)
  4. handleReport now handles the "forcefully enabled" case, by
    temporarily overriding the writeln hook with something that always
    echoes the message (this mirrors the previous behaviour of the
    report hook implementations)

In short, more responsibility regarding reports shift into a single
place (handleReport), with the report hook now only being invoked for
reports that should be displayed. Centralizing Report handling is a
step towards eventually removing/replacing Reports.

Since the the report hook used by nimsuggest doesn't consider
writeability ( writabilityKind ), this fixes all produced warnings
being
reported by chk, even those that are disabled.

Finally, disabled reports no longer triggering error handling fixes
disabled warnings promoted to errors being able to abort compilation.

Summary
=======

* fix hints/warnings promoted to errors always counting towards the
  maximum error count, even if the hint/warnings is currently disabled
* fix disabled warnings and hints being reported by `nimsuggest chk`

Details
=======

Processing of a `Report` previously worked as follows:
1. `handleReport` pre-processes the report
2. the report is passed to the report hook
3. the hook outputs the report if it determines that the report should
   be rendered and displayed
4. `handleReport` determines the error handling strategy
5. error handling is performed

This has two problems:
1. the responsibility of deciding whether a report should be output is
   left to the hook, which also means that the hook needs access to all
   state necessary to make the decision
2. error handling always takes place, even if the report is disabled

Report handling is split up, and the responsibilities are shifted:
1. if a report is disabled (`isEnabled` returns false), `handleReport`
   is a no-op
2. deciding writability is now the responsibility of `handleReport`
   (i.e., the compiler), with the report hook only invoked if the
   report is eligible for being displayed
3. computing writability no longer takes into account whether the
   report is enabled (since writeability is only decided *after* it is
   know that a report is enabled)
4. `handleReport` now handles the "forcefully enabled" case, by
   temporarily overriding the `writeln` hook with something that always
   echoes the message (this mirrors the previous behaviour of the
   report hook implementations)

In short, more responsibility regarding reports shift into a single
place (`handleReport`), with the report hook now only being invoked for
reports that should be displayed. Centralizing `Report` handling is a
step towards eventually removing/replacing `Report`s.

Since the the report hook used by `nimsuggest` doesn't consider
writeability (`writabilityKind`), this fixes all produced warnings being
reported by `chk`, even those that are disabled.

Finally, disabled reports no longer triggering error handling fixes
disabled warnings promoted to errors being able to abort compilation.
The LineTooLong hints are no longer reported, as it should be.
@zerbina zerbina added bug Something isn't working compiler/msgs Compiler output and diagnostic subsystem: error and warnig reporting, information, debugging labels Oct 7, 2023
@zerbina zerbina added this to the Diagnostics and error messages milestone Oct 7, 2023
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix, and also glad it cuts away more of the report hook mess.

@saem
Copy link
Collaborator

saem commented Oct 8, 2023

/merge

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

  • discovered while trying to enable --warningAsError:UnusedImport for the CI
  • this seems to be a regression from Structured reports #94

@chore-runner chore-runner bot added this pull request to the merge queue Oct 8, 2023
Merged via the queue into nim-works:devel with commit 434604c Oct 8, 2023
18 checks passed
@zerbina zerbina deleted the msgs-more-decision-making-in-handleReport branch October 12, 2023 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/msgs Compiler output and diagnostic subsystem: error and warnig reporting, information, debugging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants