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

Warning and Hint Pragmas do not print to console when declared from a std lib module #12424

Open
rayman22201 opened this issue Oct 12, 2019 · 6 comments

Comments

@rayman22201
Copy link
Contributor

{.Warning.} and {.Hint.} pragmas that are declared inside std lib modules are ignored and not displayed to the user when compiling.

The same pragmas in user code display correctly during compilation.

Note:
{.error.} and {.deprecated.} work correctly in std lib modules.

According to @krux02:

it probably has to do with the fact that many messages from the standard library are prohibited. Pretty annoying if you ask me. It should be to not bother user with internal warning of the standard library, but it also prevents me from actually seeing them in the first place so they can be fixed.

Example

Here is an example of why this is not a good design:

This PR had to be reverted because it broke behavior for some users:
#12231

TLDR; the PR added a {.error.} pragma to the locks.nim module if threads are not enabled.

I attempted to re-implement the PR using a {.Warning.} pragma as a compromise.
Unfortunately, converting the message to a {.Warning.} caused Nim to completely ignore the message.

This is a completely valid case for a std lib module providing a useful warning to the user, and not just noise from the std lib internals.

@krux02
Copy link
Contributor

krux02 commented Oct 13, 2019

I would also like to mention that it became annoying for me when working for the std lib module because this behavior actively hid warnings problems from me that I was trying to find and fix.

@rayman22201
Copy link
Contributor Author

I would like to note that I am game for doing the work to change this behavior, but I need help finding where to start. I just did a quick search, but I was not able to find the code where this is happening.

@Araq
Copy link
Member

Araq commented Oct 15, 2019

In compiler/modules.nim there is this piece of code:

  graph.config.notes =
    if s.owner.id == graph.config.mainPackageId or isDefined(graph.config, "booting"): graph.config.mainPackageNotes
    else: graph.config.foreignPackageNotes

So you can enable these already via -d:booting but of course a better solution would be nice.

@rayman22201
Copy link
Contributor Author

rayman22201 commented Oct 16, 2019

Two ideas:

1.) Get rid of that -d:booting condition and just always return graph.config.mainPackageNotes
Seems like what @krux02 would like. But idk the consequences

2.) Create a new pragma like {.externalWarning: "string"} that is enabled in graph.config.foreignPackageNotes

I don't love the idea of adding a new pragma, but maybe that's the safest thing to do?

Thoughts?

@disruptek
Copy link
Contributor

//cc @narimiran

@ghost
Copy link

ghost commented Mar 18, 2021

Fixed by #17024, needs a test case I guess.

Araq pushed a commit that referenced this issue Aug 30, 2024
closes #1969, closes #7547, closes #7737, closes #11838, closes #12283,
closes #12714, closes #12720, closes #14053, closes #16118, closes
#19670, closes #22645

I was going to wait on these but regression tests even for recent PRs
are turning out to be important in wide reaching PRs like #24010.

The other issues with the working label felt either finnicky (#7385,
#9156, #12732, #15247), excessive to test (#12405, #12424, #17527), or I
just don't know what fixed them/what the issue was (#16128: the PR link
gives a server error by Github, #12457, #12487).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants