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

sem, pragmas: fix fatal pragma #1386

Merged
merged 6 commits into from
Jul 22, 2024
Merged

Conversation

starsiderfox
Copy link
Contributor

@starsiderfox starsiderfox commented Jul 21, 2024

Summary

Using {.fatal:"...".} did not show any message to the user and it
didn't stop compilation.
This commit fixes both issues.

Details

  • adSemFatalError now carries a message just like
    adSemCustomUserError .
  • All reports of fatal severity abort compilation.
  • Testament now also consider fatal errors to be errors.
  • A simple test case has been included.
  • isCompilerFatal has been removed as it seems it lost its purpose.
    The check for rextCmdRequiresFile seems to be unnecessary. A fatal
    report is produced elsewhere (the only place where
    rextCmdRequiresFile is used) before this function is ever called, so
    it was redundant and it doesn't break existing code.

@zerbina zerbina added bug Something isn't working compiler/sem Related to semantic-analysis system of the compiler compiler/msgs Compiler output and diagnostic subsystem: error and warnig reporting, information, debugging labels Jul 21, 2024
Copy link
Collaborator

@zerbina zerbina left a comment

Choose a reason for hiding this comment

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

This is a very clean and concise fix. Thank you!

There are two small things that need adjustment, but otherwise this looks good.

compiler/front/cli_reporter.nim Outdated Show resolved Hide resolved
compiler/front/msgs.nim Outdated Show resolved Hide resolved
@zerbina
Copy link
Collaborator

zerbina commented Jul 21, 2024

Also, please make sure the PR message follows the guidelines. The first line works well as the summary; the details section should describe what changed internally (i.e., adSemFatalError now carries an error message, and sem reports with fatal severity are considered fatal errors).

compiler/ast/reports.nim Outdated Show resolved Hide resolved
@starsiderfox starsiderfox marked this pull request as ready for review July 21, 2024 22:19
Copy link
Collaborator

@zerbina zerbina left a comment

Choose a reason for hiding this comment

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

Apart from isFatalSeverity (refer to the comment I left regarding it), this PR is good to be merged.

Copy link
Collaborator

@zerbina zerbina left a comment

Choose a reason for hiding this comment

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

Looks good from my side, thank you.

@zerbina zerbina requested a review from saem July 21, 2024 22:57
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.

LGTM

@zerbina
Copy link
Collaborator

zerbina commented Jul 21, 2024

/merge

Copy link

Merge requested by: @zerbina

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


@chore-runner chore-runner bot added this pull request to the merge queue Jul 21, 2024
Merged via the queue into nim-works:devel with commit 4f95d2a Jul 22, 2024
31 checks passed
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 compiler/sem Related to semantic-analysis system of the compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants