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

Forbid pineapple on pizza #70645

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ mod late;
mod levels;
mod non_ascii_idents;
mod nonstandard_style;
mod nonstandard_taste;
mod passes;
mod redundant_semicolon;
mod types;
Expand All @@ -69,6 +70,7 @@ use builtin::*;
use internal::*;
use non_ascii_idents::*;
use nonstandard_style::*;
use nonstandard_taste::*;
use redundant_semicolon::*;
use types::*;
use unused::*;
Expand Down Expand Up @@ -183,6 +185,7 @@ macro_rules! late_lint_mod_passes {
UnreachablePub: UnreachablePub,
ExplicitOutlivesRequirements: ExplicitOutlivesRequirements,
InvalidValue: InvalidValue,
PineappleOnPizza: PineappleOnPizza,
]
);
};
Expand Down Expand Up @@ -247,6 +250,8 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) {
store.register_lints(&BuiltinCombinedLateLintPass::get_lints());
}

add_lint_group!("nonstandard_taste", PINEAPPLE_ON_PIZZA);

add_lint_group!(
"nonstandard_style",
NON_CAMEL_CASE_TYPES,
Expand Down
52 changes: 52 additions & 0 deletions src/librustc_lint/nonstandard_taste.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
use crate::{LateContext, LateLintPass, LintContext};
use rustc_hir as hir;

declare_lint! {
pub PINEAPPLE_ON_PIZZA,
Forbid,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs its own lint group. It should go with other similar lints, like allow(bad_style).

Copy link
Member

Choose a reason for hiding this comment

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

Not allow(bad_style), but allow(bad_taste).

Copy link

@EvanCarroll EvanCarroll Apr 1, 2020

Choose a reason for hiding this comment

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

I don't think we should allow pineapple pizza to be instantiated regardless of the pragma or directive. We shouldn't permit terrorism.

"pineapple doesn't go on pizza"
}

declare_lint_pass!(PineappleOnPizza => [PINEAPPLE_ON_PIZZA]);

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for PineappleOnPizza {
fn check_ty(&mut self, cx: &LateContext<'_, '_>, ty: &hir::Ty<'_>) {
if let hir::TyKind::Path(hir::QPath::Resolved(_, path)) = &ty.kind {
for pizza_segment in path.segments {
if let Some(args) = pizza_segment.args {
if pizza_segment.ident.name.as_str().to_lowercase() != "pizza" {
continue;
}
for arg in args.args {
if let hir::GenericArg::Type(hir::Ty {
Copy link
Member

Choose a reason for hiding this comment

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

This should be extended to support const generics:

Pizza<pineapple()>

should not be allowed, since there is no value to having pineapple on pizza.

kind: hir::TyKind::Path(hir::QPath::Resolved(_, path)),
..
}) = arg
{
for pineapple_segment in path.segments {
if pineapple_segment.ident.name.as_str().to_lowercase()
== "pineapple"
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't correctly interact with non_ascii_idents, it should also account for 🍍 and 🍕. For future compatibility it should probably also support 🍕ZWJ🍍.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also account for people try to sneak things into the codebase with 🌲ZWJ🍎.

{
cx.struct_span_lint(
PINEAPPLE_ON_PIZZA,
pineapple_segment.ident.span,
|lint| {
let mut err =
lint.build("pineapple doesn't go on pizza");
err.span_label(
pizza_segment.ident.span,
"this is the pizza you ruined",
);
err.note("you're a monster"); // Yep Esteban, you are.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
err.note("you're a monster"); // Yep Esteban, you are.
err.note("you're a monster");

Remove salty comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with it.

err.emit();
},
);
}
}
}
}
}
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This is way too little nesting. Please use match instead of if let and loop { match iter.next() { /*...*/ } } instead of for. Also introduce new bindings using fn r#let<T>(x: T, f: impl FnOnce(T)) { f(x); } let(foo, |bar| { /* ... */ }); instead of let.

Copy link
Contributor

Choose a reason for hiding this comment

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

what is this fn r#thing syntax and how do I read about it?

Choose a reason for hiding this comment

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

let is a keyword so if you want to call something let, you gotta use the "raw identifier" prefix like r#let.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, fancy! Thankyou!

6 changes: 6 additions & 0 deletions src/test/ui/lint/pineapple-on-pizza.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
struct Pizza<T>(T);
struct Pineapple;

fn main() {
let _: Pizza<Pineapple>; //~ERROR pineapple doesn't go on pizza
Copy link
Member

Choose a reason for hiding this comment

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

A test is necessary to ensure that this lint fires regardless of the order in which – or count of – ingredients are applied to the base.

}
13 changes: 13 additions & 0 deletions src/test/ui/lint/pineapple-on-pizza.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
error: pineapple doesn't go on pizza
--> $DIR/pineapple-on-pizza.rs:5:18
|
LL | let _: Pizza<Pineapple>;
| ----- ^^^^^^^^^
| |
| this is the pizza you ruined
|
= note: `#[forbid(pineapple_on_pizza)]` on by default
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the atrocious crime against taste that has been committed here, I wonder if we should delete the file from the user's hard-drive. Although, I'm not sure how to test that... 🤔

Copy link
Member

@phansch phansch Apr 1, 2020

Choose a reason for hiding this comment

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

We could get close by obtaining the whole span of the file and adding a removal suggestion to remove the complete span. Since we have rustfix tests this should be easily testable 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah that's clever! @pietroalbini Can you adjust the PR to do that?

Copy link
Contributor

@jcdyer jcdyer Apr 2, 2020

Choose a reason for hiding this comment

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

Can you get the span of the whole crate?

Sigourney Weaver in Aliens saying, "It's the only way to be sure."

= note: you're a monster

error: aborting due to previous error