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.

Sign up for GitHub

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

fixes #20149; fixes #16762; hintAsError and warningAsError now ignore foreign packages #20151

Merged
merged 8 commits into from
Aug 19, 2022
Merged

fixes #20149; fixes #16762; hintAsError and warningAsError now ignore foreign packages #20151

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
542d980
fixes #20149; hintAsError/warningAsError ignores foreign packages
ringabout Aug 4, 2022
b41118a
add changelog
ringabout Aug 4, 2022
278c41a
fixes the test
ringabout Aug 4, 2022
aee6be1
remove
ringabout Aug 4, 2022
bd42490
fixes tests again
ringabout Aug 4, 2022
71de68a
fix
ringabout Aug 4, 2022
2302ad8
I'm careless
ringabout Aug 4, 2022
a3dda8e
Merge branch 'devel' into pr_warnings
ringabout Aug 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@
- `nim` can now compile version 1.4.0 as follows: `nim c --lib:lib --stylecheck:off compiler/nim`,
without requiring `-d:nimVersion140` which is now a noop.

- `--styleCheck` now only applies to the current package.
- `--styleCheck`, `--hintAsError` and `--warningAsError` now only applies to the current package.


## Tool changes
Expand Down
12 changes: 5 additions & 7 deletions compiler/msgs.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,12 +418,12 @@ To create a stacktrace, rerun compilation with './koch temp $1 <file>', see $2 f
[conf.command, "intern.html#debugging-the-compiler".createDocLink], conf.unitSep)
quit 1

proc handleError(conf: ConfigRef; msg: TMsgKind, eh: TErrorHandling, s: string) =
proc handleError(conf: ConfigRef; msg: TMsgKind, eh: TErrorHandling, s: string, ignoreMsg: bool) =
if msg in fatalMsgs:
if conf.cmd == cmdIdeTools: log(s)
quit(conf, msg)
if msg >= errMin and msg <= errMax or
(msg in warnMin..hintMax and msg in conf.warningAsErrors):
(msg in warnMin..hintMax and msg in conf.warningAsErrors and not ignoreMsg):
inc(conf.errorCounter)
conf.exitcode = 1'i8
if conf.errorCounter >= conf.errorMax:
Expand Down Expand Up @@ -531,8 +531,7 @@ proc liMessage*(conf: ConfigRef; info: TLineInfo, msg: TMsgKind, arg: string,
of warnMin..warnMax:
sev = Severity.Warning
ignoreMsg = not conf.hasWarn(msg)
if msg in conf.warningAsErrors:
ignoreMsg = false
if not ignoreMsg and msg in conf.warningAsErrors:
Copy link
Contributor

Choose a reason for hiding this comment

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

What meaning does it have for msg in conf.warningAsErrors to be true? What would a boolean that contained the result of that statement be accurately be named like?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe you could expand this test with an unused import to test that in a single go? Would only require e.g. import std/options in the file and --warningAsError:UnusedImport in trunner.nim

Sounds good to me. Feel free to add a test case in the following up prs.

Is there a way to test that the flag correctly prohibits compilation? Or would that be out of scope?

Yeah, there are tests for that. Using compiles should work for simple cases. If you can also go through the trouble of using execCmdEx and compare the outputs. Following-up prs are welcome!

Copy link
Member Author

Choose a reason for hiding this comment

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

What meaning does it have for msg in conf.warningAsErrors to be true?

conf.warningAsErrors is a set of TMsgKind enum type. If msg in the conf.warningAsErrors, it means this hints/warnings are enabled for the file and you have already specified that hints/warnings should be errors.

For instance, for a module within the current package, XDeclaredButNotUsed is enabled. If you also specify --hintAsError:XDeclaredButNotUsed is true, in this case XDeclaredButNotUsed is the msg variable, --hintAsError:XDeclaredButNotUsed means XDeclaredButNotUsed is added to conf.warningAsErrors. So the statement "msg in conf.warningAsErrors" is true.

For a foreign package, XDeclaredButNotUsed is not enabled by default. Even if --hintAsError:XDeclaredButNotUsed is specified, the statement "msg in conf.warningAsErrors" is false.

title = ErrorTitle
else:
title = WarningTitle
Expand All @@ -542,8 +541,7 @@ proc liMessage*(conf: ConfigRef; info: TLineInfo, msg: TMsgKind, arg: string,
of hintMin..hintMax:
sev = Severity.Hint
ignoreMsg = not conf.hasHint(msg)
if msg in conf.warningAsErrors:
ignoreMsg = false
if not ignoreMsg and msg in conf.warningAsErrors:
title = ErrorTitle
else:
title = HintTitle
Expand All @@ -569,7 +567,7 @@ proc liMessage*(conf: ConfigRef; info: TLineInfo, msg: TMsgKind, arg: string,
" compiler msg initiated here", KindColor,
KindFormat % $hintMsgOrigin,
resetStyle, conf.unitSep)
handleError(conf, msg, eh, s)
handleError(conf, msg, eh, s, ignoreMsg)
if msg in fatalMsgs:
# most likely would have died here but just in case, we restore state
conf.m.errorOutputs = errorOutputsOld
Expand Down
2 changes: 2 additions & 0 deletions tests/misc/m20149.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
let x = 12
echo x
5 changes: 5 additions & 0 deletions tests/misc/trunner.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,11 @@ sub/mmain.idx""", context
let cmd = fmt"{nim} r -b:cpp --hints:off --nimcache:{nimcache} --warningAsError:ProveInit {file}"
check execCmdEx(cmd) == ("witness\n", 0)

block: # bug #20149
let file = testsDir / "misc/m20149.nim"
let cmd = fmt"{nim} r --hints:off --nimcache:{nimcache} --hintAsError:XDeclaredButNotUsed {file}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll still approve this, but would like to note that this will break if nim or nimcache are not properly escaped.

check execCmdEx(cmd) == ("12\n", 0)

block: # config.nims, nim.cfg, hintConf, bug #16557
let cmd = fmt"{nim} r --hint:all:off --hint:conf tests/newconfig/bar/mfoo.nim"
let (outp, exitCode) = execCmdEx(cmd, options = {poStdErrToStdOut})
Expand Down