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

Add a lint for certain suspicious trait object usage #264

Merged
merged 2 commits into from
Mar 23, 2023

Conversation

thomcc
Copy link
Contributor

@thomcc thomcc commented Mar 20, 2023

No description provided.

@thomcc
Copy link
Contributor Author

thomcc commented Mar 20, 2023

Ah wait, this might not be enough Nah it seems to be fine.

@thomcc thomcc force-pushed the thomcc/sus-trait-obj branch from 8ee6642 to fc274e0 Compare March 21, 2023 14:11
plrustc/plrustc/src/lints.rs Show resolved Hide resolved
hir::ItemKind::Union(_, generics) => *generics,
hir::ItemKind::Trait(_, _, generics, ..) => *generics,
hir::ItemKind::Fn(_, generics, ..) => *generics,
_ => return,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark as before.

Though, I'm surprised you can't leverage OpaqueTy somehow to induce this problem. Or can you and we already check that?

Copy link
Contributor Author

@thomcc thomcc Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpaqueTy is unstable, it's TAIT stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather, using OpaqueTy in a turbofish or default type requires TAIT.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh, I misread rustc's docs and lost the type Foo = part, thought for some reason that all impl Trait goes through it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh oh oh, so wait... wait... OpaqueTy is a threat but it's currently syntactically inaccessible in this position, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not sure that struct/enum/type/... are a threat in the first place. They're easy to check for and it feels like I almost got it working, so I figured I'd check for them.

But yeah, TraitAlias is probably in the same position. I could put it here too, but this whole thing probably needs rethinking when those stabilize

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've filed #270 about this kind of thing.

@workingjubilee workingjubilee merged commit 9641933 into main Mar 23, 2023
@thomcc thomcc deleted the thomcc/sus-trait-obj branch March 23, 2023 22:02
@eeeebbbbrrrr eeeebbbbrrrr mentioned this pull request Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants