Warn if include macro fails to include entire file #64284

Mark-Simulacrum · 2019-09-08T14:03:40Z

This currently introduces an error, mainly because that was just simpler, and I'm not entirely certain if we can introduce a lint without an RFC and such.

This is primarily to get feedback on the approach and overall aim -- in particular, do we think this is helpful? If so, we probably will need lang-team sign off and decide if it should be an error (as currently introduced by this PR), a lint, or a warning.

r? @petrochenkov

cc #35560

src/libsyntax_ext/source_util.rs

Centril · 2019-09-08T14:40:27Z

Ostensibly we could do what the user wants by switching to parsing a block implicitly and putting a bunch of statements in there. On the other hand, this might actually change the dynamic semantics of some programs (but that's probably a bug-fix if it does...).

Mark-Simulacrum · 2019-09-08T15:31:29Z

As far as I know, we can't just switch to parsing it as a block of statements because of how macro expansion works, but I could well be wrong about that.

petrochenkov · 2019-09-08T17:55:00Z

Seems like an obvious bugfix to me (compare with with fn make_items below which does perform the token::Eof check for item position include!s).

The end goal is to migrate built-in macros to the token-based model (which is currently in effect for proc macros).
In that model the macro returns tokens, and those tokens are parsed into an AST fragment depending on the macro's position (expresstion, type, items or whatever). If the tokens don't fit into that fragment's grammar, then it's an error. (In this particular case the tokens don't fit into expression grammar.)

Why include!("include-single-expr-helper.rs"); is treated as an expression position macro and not a statement position macro is an separate question though, probably related to #61733.
It should be addressed by the upcoming invocation collector rewrite.
I'd expect include-single-expr.rs to compile successfully and expand into three statements.

Mark-Simulacrum · 2019-09-08T18:10:36Z

Yeah, I agree that the specific test case might not be quite ideal -- open to suggestions, I'm not sure how to structure a good test case. Maybe just 10 10 10 would be fine since it's multiple expressions (and invalid Rust grammar, I believe, as such, since expressions need some delimiter).

petrochenkov · 2019-09-09T16:50:24Z

Maybe just 10 10 10 would be fine since it's multiple expressions

I'd change the test to something like that, yes.

joshtriplett · 2019-09-12T18:50:23Z

I can see two reasonable behaviors here:

Error (not warn/lint) if we don't parse and include the entire file. (Note, though, that comments shouldn't cause an error here.)
Actually parse and include the entire file.

nikomatsakis · 2019-09-12T18:50:45Z

@Mark-Simulacrum

do we have a test for trailing comments -- can we add a test for a file like

22; /* this is a comment */

I don't think this should error. I'm not sure whether it will or not, I think it will actually work.

Mark-Simulacrum · 2019-09-12T20:08:07Z

I think we don't formally have such a test but I believe I locally tested that behavior (and can/will add such a test).

Did lang team get a chance to discuss this yet? I'd prefer to avoid throwing work at it if there's no need to (if lang team decides we're not ready to turn this into an error/warning).

pnkfelix · 2019-09-19T10:00:05Z

The rough consensus from the lang team meeting was that we should probably change the behavior.

In the long term, we probably want to error if there are extra tokens (beyond the first expression) in the file.
- We didn't discuss trailing comments specifically, to my knowledge, but the use of the language "tokens" and not "characters"/"text" in the notes leads me to think that trailing comments and other whitespace-like things should be acceptable on its own.
In the short term, we should probably implement this as a future-compat warning, to reduce risk of injecting breakage for clients of the macro.

Mark-Simulacrum · 2019-09-19T10:26:51Z

I will work on getting the tests in this PR updated and the error moved to a future compat warning (I imagine that might be a bit hard given we're pre-Session and such, but presumably worst case we just always emit it or so...). Since it's a future compat warning I'll file a tracking issue for moving it over to a hard error in the future.

Do we want to do a crater run first before we move to that? To get an initial idea of breakage?

Mark-Simulacrum · 2019-09-19T10:27:24Z

@bors try

Let's actually get a try build here on this commit and then that way we can always en-queue crater later with the commit results

bors · 2019-09-19T10:27:37Z

⌛ Trying commit 2a92dc79aae323df9f18f754690a82735d89aab5 with merge c3890be40d6e59e9a4c95121a59f6e1715a3df20...

pnkfelix · 2019-09-19T10:38:05Z

Do we want to do a crater run first before we move to that? To get an initial idea of breakage?

That seems prudent. This bug is long-standing (right?), so we don't need to rush a PR in for a backport or anything.

Mark-Simulacrum · 2019-09-19T10:48:31Z

Yes, this bug has existed since 2016 and likely much earlier.

bors · 2019-09-19T14:18:31Z

☀️ Try build successful - checks-azure
Build commit: c3890be40d6e59e9a4c95121a59f6e1715a3df20

Mark-Simulacrum · 2019-09-19T15:17:55Z

@craterbot run mode=check-only

Mark-Simulacrum · 2019-10-04T00:09:43Z

All the cases are trailing semicolons (i.e., include! in an expression context with the file containing ...;) - no false positives.

vga-framebuffer 0.7.5 cc @thejpster -- this is an old version, master no longer has include!
polylabel 1.3.0 (Remove stray semicolon urschrei/polylabel-rs#2)
adjective_adjective_animal 0.1.0 (Remove stray semicolon moparisthebest/adjective-adjective-animal#1)
https://github.com/harindaka/rust-build-script-example ( Remove stray semicolon harindaka/rust-build-script-example#1)
https://github.com/jalexvig/proji ( Remove stray semicolon jalexvig/proji#4)
names 0.11.0 ( Remove stray semicolon fnichol/names#3)
dynasm 0.5.0 (Remove stray semicolon CensoredUsername/dynasm-rs#42)
geo 0.12.2 (Remove stray semicolon georust/geo#386)

I am not sure that it is wise to land this as an error since that will break people in practice, but I think a non-allowable warning (i.e., span_warn or so) would make sense since we believe all cases to not be false positive.

petrochenkov · 2019-10-04T08:13:18Z

Hard-coded warnings are generally discouraged (and existing ones need to be converted to lints or errors), I'd personally went with a deny-by-default deprecation lint.

386: Remove stray semicolon r=urschrei a=Mark-Simulacrum This semicolon is currently silently ignored, but we may be changing that in the future. cc rust-lang/rust#64284 Co-authored-by: Mark Rousskov <mark.simulacrum@gmail.com>

Mark-Simulacrum · 2019-10-04T11:18:04Z

Am I correct that I'll need to add a lint here https://github.com/rust-lang/rust/blob/master/src/libsyntax/early_buffered_lints.rs and then thread that through into librustc? A bit annoying to need to use that infrastructure but I guess it's not too bad...

Mark-Simulacrum · 2019-10-07T23:49:11Z

r? @petrochenkov

Pushed up patch which switches to using a lint and adds a test case which should not emit a lint, despite having trailing characters in the form of a comment. Also amends the preexisting test case to have newline delimited instead of ; per earlier discussion.

petrochenkov · 2019-10-08T14:39:01Z

@bors r+

bors · 2019-10-08T14:39:03Z

📌 Commit e068cec has been approved by petrochenkov

bors · 2019-10-08T21:12:18Z

⌛ Testing commit e068cec with merge 724316ff988efeb77ce732b1f9809cf1afaeb169...

@petrochenkov

…rochenkov Warn if include macro fails to include entire file This currently introduces an error, mainly because that was just simpler, and I'm not entirely certain if we can introduce a lint without an RFC and such. This is primarily to get feedback on the approach and overall aim -- in particular, do we think this is helpful? If so, we probably will need lang-team sign off and decide if it should be an error (as currently introduced by this PR), a lint, or a warning. r? @petrochenkov cc rust-lang#35560

Centril · 2019-10-08T21:32:02Z

@bors retry rolled up.

@ghost

Rollup of 7 pull requests Successful merges: - #64284 (Warn if include macro fails to include entire file) - #65081 (Remove -Zprofile-queries) - #65133 (typeck: prohibit foreign statics w/ generics) - #65135 (Add check for missing tests for error codes) - #65141 (Replace code of conduct with link) - #65194 (Use structured suggestion for removal of `as_str()` call) - #65213 (Ignore `ExprKind::DropTemps` for some ref suggestions) Failed merges: r? @ghost

bors · 2019-10-09T01:40:57Z

⌛ Testing commit e068cec with merge b5bd31e...

Version 1.40.0 (2019-12-19) =========================== Language -------- - [You can now use tuple `struct`s and tuple `enum` variant's constructors in `const` contexts.][65188] e.g. ```rust pub struct Point(i32, i32); const ORIGIN: Point = { let constructor = Point; constructor(0, 0) }; ``` - [You can now mark `struct`s, `enum`s, and `enum` variants with the `#[non_exhaustive]` attribute to indicate that there may be variants or fields added in the future.][64639] For example this requires adding a wild-card branch (`_ => {}`) to any match statements on a non-exhaustive `enum`. [(RFC 2008)] - [You can now use function-like procedural macros in `extern` blocks and in type positions.][63931] e.g. `type Generated = macro!();` - [Function-like and attribute procedural macros can now emit `macro_rules!` items, so you can now have your macros generate macros.][64035] - [The `meta` pattern matcher in `macro_rules!` now correctly matches the modern attribute syntax.][63674] For example `(#[$m:meta])` now matches `#[attr]`, `#[attr{tokens}]`, `#[attr[tokens]]`, and `#[attr(tokens)]`. Compiler -------- - [Added tier 3 support\* for the `thumbv7neon-unknown-linux-musleabihf` target.][66103] - [Added tier 3 support for the `aarch64-unknown-none-softfloat` target.][64589] - [Added tier 3 support for the `mips64-unknown-linux-muslabi64`, and `mips64el-unknown-linux-muslabi64` targets.][65843] \* Refer to Rust's [platform support page][forge-platform-support] for more information on Rust's tiered platform support. Libraries --------- - [The `is_power_of_two` method on unsigned numeric types is now a `const` function.][65092] Stabilized APIs --------------- - [`BTreeMap::get_key_value`] - [`HashMap::get_key_value`] - [`Option::as_deref_mut`] - [`Option::as_deref`] - [`Option::flatten`] - [`UdpSocket::peer_addr`] - [`f32::to_be_bytes`] - [`f32::to_le_bytes`] - [`f32::to_ne_bytes`] - [`f64::to_be_bytes`] - [`f64::to_le_bytes`] - [`f64::to_ne_bytes`] - [`f32::from_be_bytes`] - [`f32::from_le_bytes`] - [`f32::from_ne_bytes`] - [`f64::from_be_bytes`] - [`f64::from_le_bytes`] - [`f64::from_ne_bytes`] - [`mem::take`] - [`slice::repeat`] - [`todo!`] Cargo ----- - [Cargo will now always display warnings, rather than only on fresh builds.][cargo/7450] - [Feature flags (except `--all-features`) passed to a virtual workspace will now produce an error.][cargo/7507] Previously these flags were ignored. - [You can now publish `dev-dependencies` without including a `version`.][cargo/7333] Misc ---- - [You can now specify the `#[cfg(doctest)]` attribute to include an item only when running documentation tests with `rustdoc`.][63803] Compatibility Notes ------------------- - [As previously announced, any previous NLL warnings in the 2015 edition are now hard errors.][64221] - [The `include!` macro will now warn if it failed to include the entire file.][64284] The `include!` macro unintentionally only includes the first _expression_ in a file, and this can be unintuitive. This will become either a hard error in a future release, or the behavior may be fixed to include all expressions as expected. - [Using `#[inline]` on function prototypes and consts now emits a warning under `unused_attribute` lint.][65294] Using `#[inline]` anywhere else inside traits or `extern` blocks now correctly emits a hard error. [65294]: rust-lang/rust#65294 [66103]: rust-lang/rust#66103 [65843]: rust-lang/rust#65843 [65188]: rust-lang/rust#65188 [65092]: rust-lang/rust#65092 [64589]: rust-lang/rust#64589 [64639]: rust-lang/rust#64639 [64221]: rust-lang/rust#64221 [64284]: rust-lang/rust#64284 [63931]: rust-lang/rust#63931 [64035]: rust-lang/rust#64035 [63674]: rust-lang/rust#63674 [63803]: rust-lang/rust#63803 [cargo/7450]: rust-lang/cargo#7450 [cargo/7507]: rust-lang/cargo#7507 [cargo/7525]: rust-lang/cargo#7525 [cargo/7333]: rust-lang/cargo#7333 [(rfc 2008)]: https://rust-lang.github.io/rfcs/2008-non-exhaustive.html [`f32::to_be_bytes`]: https://doc.rust-lang.org/std/primitive.f32.html#method.to_be_bytes [`f32::to_le_bytes`]: https://doc.rust-lang.org/std/primitive.f32.html#method.to_le_bytes [`f32::to_ne_bytes`]: https://doc.rust-lang.org/std/primitive.f32.html#method.to_ne_bytes [`f64::to_be_bytes`]: https://doc.rust-lang.org/std/primitive.f64.html#method.to_be_bytes [`f64::to_le_bytes`]: https://doc.rust-lang.org/std/primitive.f64.html#method.to_le_bytes [`f64::to_ne_bytes`]: https://doc.rust-lang.org/std/primitive.f64.html#method.to_ne_bytes [`f32::from_be_bytes`]: https://doc.rust-lang.org/std/primitive.f32.html#method.from_be_bytes [`f32::from_le_bytes`]: https://doc.rust-lang.org/std/primitive.f32.html#method.from_le_bytes [`f32::from_ne_bytes`]: https://doc.rust-lang.org/std/primitive.f32.html#method.from_ne_bytes [`f64::from_be_bytes`]: https://doc.rust-lang.org/std/primitive.f64.html#method.from_be_bytes [`f64::from_le_bytes`]: https://doc.rust-lang.org/std/primitive.f64.html#method.from_le_bytes [`f64::from_ne_bytes`]: https://doc.rust-lang.org/std/primitive.f64.html#method.from_ne_bytes [`option::flatten`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.flatten [`option::as_deref`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.as_deref [`option::as_deref_mut`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.as_deref_mut [`hashmap::get_key_value`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.get_key_value [`btreemap::get_key_value`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.get_key_value [`slice::repeat`]: https://doc.rust-lang.org/std/primitive.slice.html#method.repeat [`mem::take`]: https://doc.rust-lang.org/std/mem/fn.take.html [`udpsocket::peer_addr`]: https://doc.rust-lang.org/std/net/struct.UdpSocket.html#method.peer_addr [`todo!`]: https://doc.rust-lang.org/std/macro.todo.html

rust-highfive assigned petrochenkov Sep 8, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 8, 2019

Centril reviewed Sep 8, 2019

View reviewed changes

src/libsyntax_ext/source_util.rs Outdated Show resolved Hide resolved

Centril added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Sep 8, 2019

This comment has been minimized.

Sign in to view

Mark-Simulacrum force-pushed the include-warn branch from 1eb6922 to 2a92dc7 Compare September 8, 2019 17:28

petrochenkov approved these changes Sep 9, 2019

View reviewed changes

petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 9, 2019

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 19, 2019

This was referenced Oct 3, 2019

Remove stray semicolon CensoredUsername/dynasm-rs#42

Merged

Remove stray semicolon georust/geo#386

Merged

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

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 Oct 4, 2019

Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 7, 2019

Warn if include macro fails to include entire file

e068cec

Mark-Simulacrum force-pushed the include-warn branch from 2a92dc7 to e068cec Compare October 7, 2019 23:48

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 Oct 8, 2019

Centril mentioned this pull request Oct 8, 2019

Rollup of 7 pull requests #65223

Merged

bors merged commit e068cec into rust-lang:master Oct 9, 2019

soc mentioned this pull request Jan 14, 2020

Update Rust toolchain to nightly-2020-01-08 dinfuehr/dora#132

Merged

est31 mentioned this pull request Jul 14, 2021

Only the first expression in an include!d file is included. #35560

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn if include macro fails to include entire file #64284

Warn if include macro fails to include entire file #64284

Mark-Simulacrum commented Sep 8, 2019

Centril commented Sep 8, 2019

Mark-Simulacrum commented Sep 8, 2019

This comment has been minimized.

petrochenkov commented Sep 8, 2019

Mark-Simulacrum commented Sep 8, 2019

petrochenkov commented Sep 9, 2019

joshtriplett commented Sep 12, 2019

nikomatsakis commented Sep 12, 2019

Mark-Simulacrum commented Sep 12, 2019

pnkfelix commented Sep 19, 2019 •

edited

Loading

Mark-Simulacrum commented Sep 19, 2019

Mark-Simulacrum commented Sep 19, 2019

bors commented Sep 19, 2019

pnkfelix commented Sep 19, 2019

Mark-Simulacrum commented Sep 19, 2019

bors commented Sep 19, 2019

Mark-Simulacrum commented Sep 19, 2019

Mark-Simulacrum commented Oct 4, 2019

petrochenkov commented Oct 4, 2019 •

edited

Loading

Mark-Simulacrum commented Oct 4, 2019

Mark-Simulacrum commented Oct 7, 2019

petrochenkov commented Oct 8, 2019

bors commented Oct 8, 2019

bors commented Oct 8, 2019

Centril commented Oct 8, 2019

bors commented Oct 9, 2019

Warn if include macro fails to include entire file #64284

Warn if include macro fails to include entire file #64284

Conversation

Mark-Simulacrum commented Sep 8, 2019

Centril commented Sep 8, 2019

Mark-Simulacrum commented Sep 8, 2019

This comment has been minimized.

petrochenkov commented Sep 8, 2019

Mark-Simulacrum commented Sep 8, 2019

petrochenkov commented Sep 9, 2019

joshtriplett commented Sep 12, 2019

nikomatsakis commented Sep 12, 2019

Mark-Simulacrum commented Sep 12, 2019

pnkfelix commented Sep 19, 2019 • edited Loading

Mark-Simulacrum commented Sep 19, 2019

Mark-Simulacrum commented Sep 19, 2019

bors commented Sep 19, 2019

pnkfelix commented Sep 19, 2019

Mark-Simulacrum commented Sep 19, 2019

bors commented Sep 19, 2019

Mark-Simulacrum commented Sep 19, 2019

Mark-Simulacrum commented Oct 4, 2019

petrochenkov commented Oct 4, 2019 • edited Loading

Mark-Simulacrum commented Oct 4, 2019

Mark-Simulacrum commented Oct 7, 2019

petrochenkov commented Oct 8, 2019

bors commented Oct 8, 2019

bors commented Oct 8, 2019

Centril commented Oct 8, 2019

bors commented Oct 9, 2019

pnkfelix commented Sep 19, 2019 •

edited

Loading

petrochenkov commented Oct 4, 2019 •

edited

Loading