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

extract pattern lint from `non_upper_case_globals` #56478

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@euclio
Contributor

euclio commented Dec 3, 2018

This PR extracts a new lint misleading_constant_patterns from the existing non_upper_case_globals lint. The new lint is also part of the nonstandard_style lint group.

The motivation for this change is twofold: first, we'd like to move the identifier-checking nonstandard_style lints to be early lint passes, but this lint must be a late pass. Second, from issues like #25207 and #39371, it's clear that there is some desire for this part of the lint to be configured independently. I also believe that this lint is fundamentally checking something different from the non_upper_case_globals lint, so this lint may want to evolve separately.

cc #48103

r? @oli-obk

@Centril

This comment has been minimized.

Contributor

Centril commented Dec 3, 2018

Since this is merely moving around / extracting linting behavior rather than adding new behavior I don't think T-lang needs to be involved here. (cc @cramertj -- do you agree?)

@cramertj

This comment has been minimized.

Member

cramertj commented Dec 3, 2018

@Centril Yup! This seems fine to me.

@euclio euclio force-pushed the euclio:split-out-constant-pattern-lint branch from 6f0f7f8 to ab74712 Dec 3, 2018

&format!("constant pattern `{}` should be upper case", name),
)
.span_label(span, "looks like a binding")
.span_suggestion_with_applicability(

This comment has been minimized.

@oli-obk

oli-obk Dec 4, 2018

Contributor

I don't think this suggestion is useful without also changing the import or enum's name. I'd rather have this suggestion removed entirely than have it.

@@ -165,7 +166,8 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
"nonstandard_style",
NON_CAMEL_CASE_TYPES,
NON_SNAKE_CASE,
NON_UPPER_CASE_GLOBALS);
NON_UPPER_CASE_GLOBALS,
MISLEADING_CONSTANT_PATTERNS);

This comment has been minimized.

@oli-obk

oli-obk Dec 4, 2018

Contributor

cc @Manishearth

  1. what do you think of the name? It follows our rule of needing to be readable as "allow misleading constant patterns"
  2. Should this maybe just live in clippy, since it's not really a bug, but more a stylistic confusing problem

This comment has been minimized.

@euclio

euclio Dec 5, 2018

Contributor

In thinking about (2) a bit more, I do wonder if this lint really carries its weight. The rationale for the lint in #7526 is to avoid misleading patterns where constants look like bindings. However, to even get in this situation you'd have to explicitly #[allow(non_upper_case_globals)] on your const or static in the first place. For cases where you have to use a non-upper case global, then this lint really just serves as an extra hurdle that you're going to allow anyways.

I think it might suffice to include a blurb about the misleading patterns in the rationale for non_upper_case_globals: "patterns containing consts or statics that are not upper cased look like bindings, and can cause difficult-to-spot bugs". Maybe as a note?

This comment has been minimized.

@oli-obk

oli-obk Dec 6, 2018

Contributor

The issue is not declaring and using such a constant within one crate, but exporting such constants in a crate and then pattern matching on the name e.g. by accident, when you actually wanted a binding.

This comment has been minimized.

@euclio

euclio Dec 6, 2018

Contributor

Ah, that makes sense. I'll wait for @Manishearth to weigh in before making any more changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment