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

cfg attributes appear are improperly applied inside of include! #31369

Closed
sgrif opened this issue Feb 2, 2016 · 9 comments · Fixed by #33706
Closed

cfg attributes appear are improperly applied inside of include! #31369

sgrif opened this issue Feb 2, 2016 · 9 comments · Fixed by #33706
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`)

Comments

@sgrif
Copy link
Contributor

sgrif commented Feb 2, 2016

Example to reproduce:

// src/lib.rs
include!("foo.rs");

// foo.rs
#[cfg(feature = "never")]
include!("error.rs");

// error.rs
omg i'm a syntax error

This should successfully compile, but instead will have a syntax error. It should behave identically to what would occur if you inlined the first include!, (which does compile successfully)

// src/lib.rs
#[cfg(feature = "never")]
include!("error.rs");

// error.rs
omg i'm a syntax error
@durka
Copy link
Contributor

durka commented Feb 2, 2016

I think this is a duplicate of some older issues, essentially the problem is that cfg-stripping and macro expansion do not run in an unbounded loop. cfg is stripped once, then macros, then cfg again, and that's it. So macros which expand to cfg attributes do not work.

@durka
Copy link
Contributor

durka commented Feb 2, 2016

cf #22250 #30125

@sgrif
Copy link
Contributor Author

sgrif commented Feb 2, 2016

So macros which expand to cfg attributes do not work.

That isn't necessarily correct. That would mean that this would also fail:

// lib.rs
include("foo.rs");

// foo.rs
fn foo() {}

#[cfg(feature = "never")]
fn foo() {}

However, in that case the cfg attr applies completely fine.

@durka
Copy link
Contributor

durka commented Feb 2, 2016

Hmm, maybe a slightly different problem then. What doesn't work is this:

// lib.rs
include!("foo.rs");

fn main() {}

// foo.rs
fn foo() {}

#[cfg(nope)]
include!("bar.rs");

// bar.rs
fn foo() {}
bar.rs:1:1: 1:12 error: duplicate definition of value `foo` [E0428]
bar.rs:1 fn foo() {}

@sgrif
Copy link
Contributor Author

sgrif commented Feb 2, 2016

Yes. So it appears to specifically be when the cfg attr would affect whether another macro is expanded. As an additional example,

// lib.rs
include!("foo.rs");

fn main() {}

// foo.rs
fn foo() {}

#[cfg(nope)]
mod bar {
    include!("bar.rs");
}

// bar.rs
fn foo() {}

would be fine, but

// lib.rs
include!("foo.rs");

// foo.rs
#[cfg(nope)]
mod bar {
    include!("bar.rs");
}

// bar.rs
omg error

will fail.

@steveklabnik steveklabnik added the A-attributes Area: Attributes (`#[…]`, `#![…]`) label Feb 2, 2016
@durka
Copy link
Contributor

durka commented Feb 3, 2016

I think I've said this in past discussions, but if we can't fix the real issue, a band-aid would be to at least output a warning: ignoring cfg attribute or error: found cfg but it's too late (I guess it would have to be a warning for compatibility).

@sgrif
Copy link
Contributor Author

sgrif commented Apr 15, 2016

Just wanted to bump this again, as I lost over an hour to it this afternoon. Turns out struct fields aren't stripped before being passed to derive_Whatever if it goes through this case, meaning #[derive(PartialEq)] completely blows up.

Even just that warning would be a huge help -- however, the macro expansion is actually given more than sufficient information to strip the attributes on its own if need be, so I would think if we can detect that we're ignoring it, we could just fix it there.

sgrif added a commit to diesel-rs/diesel that referenced this issue Apr 15, 2016
I really dropped the ball on this one. I never should have merged in a
PR with such poor test coverage. As it turns out, this feature doesn't
even come close to working.

The `load_table_names` function did not specify the escape character for
the `LIKE` clause, meaning it was including Diesel's metadata tables.
Additionally, SQLite has several internal tables that it creates, which
also need to be excluded from schema inference.

Additionally, the types that it mapped to were far too broad, and would
lead to columns being unable to perform certain actions. Even if SQLite
treats `Text` and `VarChar` the same under the hood, we treat them
semantically differently and need to respect the users' schema.

I have changed our integration tests to make use of this feature, to
ensure it is working in the future. I had hoped to eliminate the
`backend_specific_schema` module entirely, but was unable to due to
rust-lang/rust#31369
sgrif added a commit to diesel-rs/diesel that referenced this issue Apr 15, 2016
I really dropped the ball on this one. I never should have merged in a
PR with such poor test coverage. As it turns out, this feature doesn't
even come close to working.

The `load_table_names` function did not specify the escape character for
the `LIKE` clause, meaning it was including Diesel's metadata tables.
Additionally, SQLite has several internal tables that it creates, which
also need to be excluded from schema inference.

Additionally, the types that it mapped to were far too broad, and would
lead to columns being unable to perform certain actions. Even if SQLite
treats `Text` and `VarChar` the same under the hood, we treat them
semantically differently and need to respect the users' schema.

I also learned that `PRIMARY KEY AUTOINCREMENT` doesn't imply `NOT NULL`
on SQLite. So that's fun.

I have changed our integration tests to make use of this feature, to
ensure it is working in the future. I had hoped to eliminate the
`backend_specific_schema` module entirely, but was unable to due to
rust-lang/rust#31369
@alexcrichton
Copy link
Member

In some sense I think include! has and perhaps always will be a bit of a hack in the compiler. This is likely best supported through code generation in the long term as that plumbs through the compiler much more naturally, although I'll admit to also having no idea as to what the underlying issue here is as well unfortunately :(

sgrif added a commit to diesel-rs/diesel that referenced this issue Apr 16, 2016
I really dropped the ball on this one. I never should have merged in a
PR with such poor test coverage. As it turns out, this feature doesn't
even come close to working.

The `load_table_names` function did not specify the escape character for
the `LIKE` clause, meaning it was including Diesel's metadata tables.
Additionally, SQLite has several internal tables that it creates, which
also need to be excluded from schema inference.

Additionally, the types that it mapped to were far too broad, and would
lead to columns being unable to perform certain actions. Even if SQLite
treats `Text` and `VarChar` the same under the hood, we treat them
semantically differently and need to respect the users' schema.

I also learned that `PRIMARY KEY AUTOINCREMENT` doesn't imply `NOT NULL`
on SQLite. So that's fun.

I have changed our integration tests to make use of this feature, to
ensure it is working in the future. I had hoped to eliminate the
`backend_specific_schema` module entirely, but was unable to due to
rust-lang/rust#31369
@jseyfried
Copy link
Contributor

This was fixed in #33706.

@sgrif sgrif closed this as completed Jun 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants