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

Custom derive attribute stripping is insufficient #8434

Closed
jonas-schievink opened this issue Apr 8, 2021 · 9 comments · Fixed by #16789
Closed

Custom derive attribute stripping is insufficient #8434

jonas-schievink opened this issue Apr 8, 2021 · 9 comments · Fixed by #16789
Assignees
Labels
A-macro macro expansion S-actionable Someone could pick this issue up and work on it right now

Comments

@jonas-schievink
Copy link
Contributor

jonas-schievink commented Apr 8, 2021

In

#[derive(Derive)]
#[cfg_attr(never, derive(Clone))]
struct S {
    #[cfg(never)]
    invisible_field: u8,
    #[cfg(not(never))]
    visible_field: u8,
}

The Derive macro should be passed the token stream corresponding to

struct S {
    #[cfg(not(never))]
    visible_field: u8,
}

Currently, we pass

#[cfg_attr(never, derive(Clone))]
struct S {
    #[cfg(never)]
    invisible_field: u8,
    #[cfg(not(never))]
    visible_field: u8,
}

Since we have to evalutate cfgs to do this correctly, I propose moving the remove_derive_attrs code to hir_expand.

@jonas-schievink jonas-schievink added A-macro macro expansion S-actionable Someone could pick this issue up and work on it right now labels Apr 8, 2021
@jonas-schievink jonas-schievink self-assigned this Apr 9, 2021
@jonas-schievink

This comment has been minimized.

bors bot added a commit that referenced this issue Apr 9, 2021
8443: Rewrite `#[derive]` removal code to be based on AST r=jonas-schievink a=jonas-schievink

We now remove any `#[derive]` before and including the one we want to expand, in the `macro_arg` query.

The same infra will be needed by attribute macros (except we only remove the attribute we're expanding, not any preceding ones).

Part of #8434 (doesn't implement the cfg-expansion yet, because that's more difficult)

8446: Undo path resolution hack for extern prelude r=jonas-schievink a=jonas-schievink

Reverts the change made in #7959

We don't populate the extern prelude for block DefMaps anymore,
so this is unnecessary

bors r+

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>
@jonas-schievink jonas-schievink removed their assignment Apr 9, 2021
@jonas-schievink
Copy link
Contributor Author

Another relevant upstream PR: rust-lang/rust#84110

@Arjentix
Copy link

Arjentix commented Apr 7, 2023

Hello! Our team is struggling because of this bug. Do you plan to fix this? It's open for 2 years already.

@silver-ymz
Copy link

Same problem. Is there any progress on this issue?

@jeff-hiner
Copy link

This got worse for me about 3 or 4 weeks ago, specifically with #[cfg(target_os = ...)] fields. Maybe a regression? It's unfortunately making it impossible to see real errors in VSCode.

@smalis-msft
Copy link

Any updates here? This would be a big usability boost IMO.

@izderadicka
Copy link

Same (or very similar) problem here with serde derive, it causes no-such-field error , it's caused by field, which is behind inactive feature, line 164,165. If I enable the feature in rust-analyzer settings that it's OK.
I'm almost sure that it did work some time ago - few month, so wondering why this old issue demostrated just now.

image

@ssrlive

This comment was marked as off-topic.

@wyatt-herkamp
Copy link
Contributor

I have started implementing a fix for this.
image

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macro macro expansion S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants