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

New lint rule: warn about Color constants that match existing color constants #58525

Open
jacob314 opened this issue Sep 30, 2021 · 4 comments
Open
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-lint-request P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@jacob314
Copy link
Member

jacob314 commented Sep 30, 2021

Describe the rule you'd like to see implemented
Warn if a raw color value like Color(0xFF000000) is used instead of an equivalent color constant (e.g. Colors.black)
It is fairly easy to accidentally enter a color constant that matches a color constant that is in scope for your library and features like flutter/flutter-intellij#5780 may make it more common. Using the named color constant that is already in scope makes code a bit easier to read as the semantic meaning of the color constants is clearer.

Examples

// package:flutter/material.dart
static const Color black = Color(0xFF000000);
static const Color white60 = Color(0x99FFFFFF);


// Usercode:

import 'package:flutter/material.dart';
const textColor = Color(0xFF000000); // Lint error as dupe of Color.black constant that is in scope.
const disabledColor = white60; // Not a lint error. Creating a new color name with a clearer semantic meaning is encouraged.
const textColor2 = Color.argb(255, 0, 0, 0); // Lint error as dupe of Color.black constant that is in scope.

const someIntConst = 0x99FFFFFF; // Not a lint error as not a color and want to avoid false positives.

// Usercode:

import 'package:flutter/material.dart' hide Colors;

const textColor = Color(0xFF000000); // No lint error as an existing color constant with the same value is not imported.

Example from DevTools:
https://github.com/flutter/devtools/blob/a4852d2140ebe6ee81cbd32ec7e75aa8f506a0f6/packages/devtools_app/lib/src/theme.dart#L283
Here we accidentally duplicated the Material.blue color.

Additional context
Functionality added for flutter/flutter-intellij#5780 would make it easier for users to accidentally add color values that match Material colors. Having this lint and quickfix would make it easier cleanup cases where colors that should be named are used.

A quick search of G3 suggests that this lint will catch a few cases but not so many cases that fixing the issues caught would be time consuming.

Risk: need to ensure this lint would not have undesirable false positives.

@jacob314
Copy link
Member Author

Here is an analogous CSS lint that warns about not using named colors. https://stylelint.io/user-guide/rules/list/color-named/

@pq
Copy link
Member

pq commented Sep 30, 2021

My sense is that use_named_constants should cover this case.

I'm guessing the way we're evaluating and comparing constants isn't working here?

By contrast, this does fire as expected:

const whoops = const Duration(seconds: 0); // should use Duration.zero

/fyi @a14n

@a14n
Copy link
Contributor

a14n commented Oct 1, 2021

As mentioned in dart-lang/linter#2429 (comment) the lint only checks named constants on the instantiated class (to limit false positives).

@jacob314
Copy link
Member Author

jacob314 commented Oct 1, 2021

Color is also tricky because the Color constants are technically MaterialColor objects while the duplicate user defined constants are typically just Color objects.

@devoncarew devoncarew added analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Nov 19, 2024
@devoncarew devoncarew transferred this issue from dart-lang/linter Nov 19, 2024
@pq pq added the P3 A lower priority bug or feature request label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-lint-request P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants