Disallow later override of forbid lint in same scope #73379

pnkfelix · 2020-06-15T18:21:26Z

Fix #70819

When building lint specs map for a given scope, check if forbid present on each insert.

Drive-by changes:

make LintLevelsBuilder::push private to the crate.
Add methods to LintSource for extracting its name symbol or its span.
Rewrote old test so that its use of lint attributes are consistent with what we want in the language. (Note that the fact this test existed is a slight sign that we may need a crater run on this bugfix...)

rust-highfive · 2020-06-15T18:21:29Z

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

src/librustc_middle/lint.rs

petrochenkov · 2020-06-16T20:51:16Z

Note that the fact this test existed is a slight sign that we may need a crater run on this bugfix...

The test was mostly artificial and tested behavior of attributes on function parameters rather than semantics of forbid.
It should probably be ok to defer crater checking to the regular beta run.

r=me with the nit addressed.

petrochenkov · 2020-07-26T13:16:10Z

ping @pnkfelix, this needs a rebase

…ant in the language. (Note that the fact this test existed is a slight sign that we may need a crater run on this bugfix...)

…nt on each insert. Drive-by changes: 1. make `LintLevelsBuilder::push` private to the crate. 2. Add methods to `LintSource` for extracting its name symbol or its span.

Co-authored-by: Vadim Petrochenkov <vadim.petrochenkov@gmail.com>

petrochenkov · 2020-08-18T07:58:49Z

@bors r+

bors · 2020-08-18T07:58:50Z

📌 Commit 4ec4c4f has been approved by petrochenkov

RalfJung · 2020-08-18T12:39:05Z

I think this means the hack in #70918 can now be reverted?

RalfJung · 2020-08-18T12:46:37Z

Ah, that would likely also need a change here:

rust/src/librustc_lint/levels.rs

Line 89 in 42abbd8

specs.insert(id, (level, src));

RalfJung · 2020-08-18T12:48:30Z

src/librustc_lint/levels.rs

+                    self.sess,
+                    src.span(),
+                    E0453,
+                    "{}({}) incompatible with previous forbid in same scope",


Interesting, so it is a hard error now to have forbid and something else on the same scope, but not in a nested scope?

I guess I don't care very strongly, just didn't expect this. I expected the same behavior was for nested scopes. Also I think this is inconsistent with command-line handling (where -F lint -A lint works, but the -A has no effect).

I don't understand your comment.

It is a hard error, today, to have a forbid on one scope and something else in a nested scope. (AIUI, that's the whole point of forbid, versus deny).

For example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=63cb51c86b8b1c15a8b22f132ce6c32e

Can you show me an example of what current behavior of forbid that you think I should be modelling instead? (In source code, that is; lets leave aside the semantics of command line argument parsing. where I think it is more justifiable to let -F silently override -A.)

Oops, somehow I thought forbid would overwrite later allow, I did not know that it makes them an error. Never mind then. I think my only comment for this PR then is that it would be nice to share the code that emits that error -- if the error is pre-existing; the wording is slightly different now.

I feel command-line arguments should be consistent with lint attributes though.

The problem with making command line arguments "consistent" with lint attributes, from my point of view, is that I think a lot of tools do inject (or freely mix) new command line arguments, and so I think users run into combinations of -F and -A on a single command line a lot more often than they run into #[forbid] and #[allow] in a single source item.

A hard error for mixing -F and -A when they arose due to e.g. makefile variables is really annoying for end users.

(I suppose the analogy for source code items would be macro expansion, right? Does one see #[allow] getting injected from macros very often? I can admit this has crossed my mind at times, especially back when I put in the push_unsafe/pop_unsafe hacks five years ago ...)

Indeed I'd argue that for the same reasons, a hard error could be annoying for the attributes. But then nobody complaining about it for five years is a good argument for it not being very annoying.^^

…ide-forbid-in-same-scope, r=petrochenkov Disallow later override of forbid lint in same scope Fix rust-lang#70819 When building lint specs map for a given scope, check if forbid present on each insert. Drive-by changes: 1. make `LintLevelsBuilder::push` private to the crate. 2. Add methods to `LintSource` for extracting its name symbol or its span. 3. Rewrote old test so that its use of lint attributes are consistent with what we want in the language. (Note that the fact this test existed is a slight sign that we may need a crater run on this bugfix...)

bors · 2020-08-19T06:34:37Z

⌛ Testing commit 4ec4c4f with merge fede0bb3089218672644303a714164fd8102a0c0...

JohnTitor · 2020-08-19T06:59:07Z

This needs to update the clippy test as well, see https://github.com/rust-lang-ci/rust/runs/1001525771, which is the rollup failure.

@bors retry r-

bors · 2020-08-30T17:42:36Z

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

petrochenkov · 2020-10-04T16:09:39Z

Closing due to inactivity.

…w-override-forbid-in-same-scope, r=petrochenkov Disallow overriding forbid in same scope Rebased rust-lang#73379. Fixes rust-lang#70819.

rust-highfive assigned petrochenkov Jun 15, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2020

pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 16, 2020

petrochenkov reviewed Jun 16, 2020

View reviewed changes

src/librustc_middle/lint.rs Outdated Show resolved Hide resolved

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2020

This comment has been minimized.

Sign in to view

Muirrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 16, 2020

Muirrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 13, 2020

pnkfelix and others added 4 commits August 17, 2020 23:24

rewrite old test so that its attributes are consistent with what we w…

e24aa73

…ant in the language. (Note that the fact this test existed is a slight sign that we may need a crater run on this bugfix...)

When building lint specs map for a given scope, check if forbid prese…

01e8e9f

…nt on each insert. Drive-by changes: 1. make `LintLevelsBuilder::push` private to the crate. 2. Add methods to `LintSource` for extracting its name symbol or its span.

Regression test.

ef03a5d

review suggestion: use existing defn rather than new intern call

4ec4c4f

Co-authored-by: Vadim Petrochenkov <vadim.petrochenkov@gmail.com>

pnkfelix force-pushed the issue-70819-disallow-override-forbid-in-same-scope branch from 45b5f1e to 4ec4c4f Compare August 18, 2020 05:24

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 18, 2020

RalfJung reviewed Aug 18, 2020

View reviewed changes

tmandry mentioned this pull request Aug 18, 2020

Rollup of 10 pull requests #75684

Closed

tmandry mentioned this pull request Aug 19, 2020

Rollup of 11 pull requests #75688

Closed

tmandry mentioned this pull request Aug 19, 2020

Rollup of 11 pull requests #75690

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 19, 2020

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 18, 2020

petrochenkov closed this Oct 4, 2020

petrochenkov mentioned this pull request Oct 4, 2020

forbid overwritten by later allow on the same "scope level" #70819

Closed

Mark-Simulacrum mentioned this pull request Oct 4, 2020

Disallow overriding forbid in same scope #77534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow later override of forbid lint in same scope #73379

Disallow later override of forbid lint in same scope #73379

pnkfelix commented Jun 15, 2020 •

edited

Loading

rust-highfive commented Jun 15, 2020

petrochenkov commented Jun 16, 2020

This comment has been minimized.

petrochenkov commented Jul 26, 2020

petrochenkov commented Aug 18, 2020

bors commented Aug 18, 2020

RalfJung commented Aug 18, 2020

RalfJung commented Aug 18, 2020 •

edited

Loading

RalfJung Aug 18, 2020

pnkfelix Aug 18, 2020 •

edited

Loading

RalfJung Aug 19, 2020

pnkfelix Aug 19, 2020

RalfJung Aug 19, 2020

bors commented Aug 19, 2020

JohnTitor commented Aug 19, 2020

bors commented Aug 30, 2020

petrochenkov commented Oct 4, 2020

Disallow later override of forbid lint in same scope #73379

Disallow later override of forbid lint in same scope #73379

Conversation

pnkfelix commented Jun 15, 2020 • edited Loading

rust-highfive commented Jun 15, 2020

petrochenkov commented Jun 16, 2020

This comment has been minimized.

petrochenkov commented Jul 26, 2020

petrochenkov commented Aug 18, 2020

bors commented Aug 18, 2020

RalfJung commented Aug 18, 2020

RalfJung commented Aug 18, 2020 • edited Loading

RalfJung Aug 18, 2020

Choose a reason for hiding this comment

pnkfelix Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

RalfJung Aug 19, 2020

Choose a reason for hiding this comment

pnkfelix Aug 19, 2020

Choose a reason for hiding this comment

RalfJung Aug 19, 2020

Choose a reason for hiding this comment

bors commented Aug 19, 2020

JohnTitor commented Aug 19, 2020

bors commented Aug 30, 2020

petrochenkov commented Oct 4, 2020

pnkfelix commented Jun 15, 2020 •

edited

Loading

RalfJung commented Aug 18, 2020 •

edited

Loading

pnkfelix Aug 18, 2020 •

edited

Loading