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

Broken move semantics in decl_macro #46314

Closed
valff opened this issue Nov 27, 2017 · 6 comments · Fixed by #49718
Closed

Broken move semantics in decl_macro #46314

valff opened this issue Nov 27, 2017 · 6 comments · Fixed by #49718
Assignees
Labels
A-borrow-checker Area: The borrow checker A-macros-2.0 Area: Declarative macros 2.0 (#39412) A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. P-medium Medium priority

Comments

@valff
Copy link
Contributor

valff commented Nov 27, 2017

Move semantics with declarative macros 2.0 are broken. Can copy non-copy types.
Playground

#![feature(decl_macro)]
#![allow(unused_macros)]

#[derive(Debug)]
struct NonCopy(String);

struct Foo {
  x: NonCopy,
}

macro copy($foo:ident) {
  $foo.x
}

macro_rules! copy2 {
  ($foo:ident) => {
    $foo.x
  }
}

fn main() {
  let foo = Foo { x: NonCopy("foo".into()) };
  assert_two_copies(copy!(foo), foo.x);
  // A similar old-style macro will not work:
  //assert_two_copies(copy2!(foo), foo.x);
}

fn assert_two_copies(a: NonCopy, b: NonCopy) {
  println!("Got two copies: {:?}, {:?}", a, b);
}

rustc --version --verbose:

rustc 1.23.0-nightly (63739ab7b 2017-11-21)
binary: rustc
commit-hash: 63739ab7b210c1a8c890c2ea5238a3284877daa3
commit-date: 2017-11-21
host: x86_64-unknown-linux-gnu
release: 1.23.0-nightly
LLVM version: 4.0
@kennytm kennytm added A-macros-2.0 Area: Declarative macros 2.0 (#39412) C-bug Category: This is a bug. labels Nov 27, 2017
@kennytm
Copy link
Member

kennytm commented Nov 27, 2017

The error is caught when enabling MIR borrow-check (pass -Z borrowck-mir or -Z nll), so this is a HIR borrow-check-only issue.

@kennytm kennytm added the A-borrow-checker Area: The borrow checker label Nov 27, 2017
@petrochenkov
Copy link
Contributor

Probably some fields are compared by name without hygiene adjustments.

@jseyfried jseyfried self-assigned this Dec 1, 2017
@pnkfelix pnkfelix added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll labels Dec 21, 2017
@pnkfelix
Copy link
Member

(tagging with nll labels just so we remember to look at this case when double checking lexical vs NLL.)

@nikomatsakis
Copy link
Contributor

We should file this under #47366, seems like?

@petrochenkov
Copy link
Contributor

It's very likely this is fixed on my field hygiene branch, I'll check.

@petrochenkov petrochenkov self-assigned this Apr 3, 2018
@valff
Copy link
Contributor Author

valff commented Apr 3, 2018

@nikomatsakis Thanks for the reference. I will make a PR with a test for this bug soon.

valff added a commit to valff/rust that referenced this issue Apr 4, 2018
kennytm added a commit to kennytm/rust that referenced this issue Apr 4, 2018
…komatsakis

Regression test for rust-lang#46314

rust-lang#46314 is fixed by NLL. This PR adds a regression test for the bug. Intended for rust-lang#47366.
bors added a commit that referenced this issue Apr 4, 2018
Rollup of 25 pull requests

Successful merges:

 - #49179 (Handle future deprecation annotations )
 - #49512 (Add support for variant and types fields for intra links)
 - #49515 (fix targetted value background)
 - #49516 (Add missing anchor for union type fields)
 - #49532 (Add test for rustdoc ignore test)
 - #49533 (Add #[must_use] to a few standard library methods)
 - #49540 (Fix miri Discriminant() for non-ADT)
 - #49559 (Introduce Vec::resize_with method (see #41758))
 - #49570 (avoid IdxSets containing garbage above the universe length)
 - #49577 (Stabilize String::replace_range)
 - #49599 (Fix typo)
 - #49603 (Fix url for intra link provided method)
 - #49607 (Stabilize iterator methods in 1.27)
 - #49609 (run-pass/attr-stmt-expr: expand test cases)
 - #49612 (Fix "since" version for getpid feature.)
 - #49618 (Fix build error when compiling libcore for 16bit targets)
 - #49619 (tweak core::fmt docs)
 - #49637 (Stabilize parent_id())
 - #49639 (Update Cargo)
 - #49628 (Re-write the documentation index)
 - #49594 (Add some performance guidance to std::fs and std::io docs)
 - #49625 (miri: add public alloc_kind accessor)
 - #49634 (Add a test for the fix to issue #43058)
 - #49641 (Regression test for #46314)
 - #49547 (Unignore borrowck test)

Failed merges:
bors added a commit that referenced this issue Apr 13, 2018
Hygiene 2.0: Avoid comparing fields by name

There are two separate commits here (not counting tests):
- The first one unifies named (`obj.name`) and numeric (`obj.0`) field access expressions in AST and HIR. Before field references in these expressions are resolved it doesn't matter whether the field is named or numeric (it's just a symbol) and 99% of code is common. After field references are resolved we work with
them by index for all fields (see the second commit), so it's again not important whether the field was named or numeric (this includes MIR where all fields were already by index).
(This refactoring actually fixed some bugs in HIR-based borrow checker where borrows through names (`S {
0: ref x }`) and indices (`&s.0`) weren't considered overlapping.)
- The second commit removes all by-name field comparison and instead resolves field references to their indices  once, and then uses those resolutions. (There are still a few name comparisons in save-analysis, because save-analysis is weird, but they are made correctly hygienic).
Thus we are fixing a bunch of "secondary" field hygiene bugs (in borrow checker, lints).

Fixes #46314
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker A-macros-2.0 Area: Declarative macros 2.0 (#39412) A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. P-medium Medium priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants