Skip to content

proposal: consider_const_constructor #59001

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

Open
5 tasks done
KyleFin opened this issue Jan 10, 2023 · 2 comments · Fixed by dart-lang/language#4035
Open
5 tasks done

proposal: consider_const_constructor #59001

KyleFin opened this issue Jan 10, 2023 · 2 comments · Fixed by dart-lang/language#4035
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-linter Issues with the analyzer's support for the linter package linter-lint-proposal linter-set-flutter linter-status-pending P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@KyleFin
Copy link

KyleFin commented Jan 10, 2023

consider_const_constructor

Description

Classes with no non-final fields may benefit from defining a const constructor.

Details

https://dart.dev/guides/language/effective-dart/design#consider-making-your-constructor-const-if-the-class-supports-it

Kind

Suggest potential optimization. Make authors deliberately choose to use const OR ignore lint. Point to useful Effective Dart reference to help them decide.

Bad Examples

class MyException extends Exception {}
class Square {
  Square(this.sideLength);
  
  Square.fromRadius(double radius) : sideLength = 2 * radius;

  final double sideLength;
}
class Square {
  Square(this.sideLength, this.color);

  final double sideLength;
  final Color color;
}

Good Examples

// Add a const constructor
class MyException extends Exception {
  const MyException();
}
// Make an existing constructor const
class Square {
  Square(this.sideLength);
  
  const Square.fromRadius(double radius) : sideLength = 2 * radius;

  final double sideLength;
}
// Explicitly ignore lint
class Square {
  // ignore: consider_const_constructor
  // Reason: we may someday change color to a getter.
  Square(this.sideLength, this.color);

  final double sideLength;
  final Color color;
}

Discussion

Background

I came upon this when reviewing some code using bloc. I assumed the default constructor was const and almost suggested removing the const constructor from an event class:

class TimerEnded extends EndingEvent {
  const TimerEnded();
}

// What I almost suggested:
class TimerEnded extends EndingEvent {}

I investigated more and learned that the default constructor is not const. Some resources I came across that helped me better understand why that's the case and when to use or not use const include:

Considerations

  • Should lint just check that at least one const constructor exists? Or that all defined constructors are const?

    • In Square.fromRadius example above, should making one constructor const satisfy the lint check, or should both constructors be required to be made const (or ignored)?
    • Does lint / ignore apply to individual constructors or to the entire class? Or both?
      • I'm not sure, but I think the check would apply to each constructor and we could have individual or entire class ignores.
  • We could have a quick fix to create const constructor (if no constructors are defined) or make an existing constructor const, but it may be better to NOT have a quick fix so authors don't accidentally agree to maintaining a const constructor in their public API.

    • The intent of this lint is to remind authors to be intentional and guide them in deciding whether or not they want to use const.
  • Is it worth having this lint?

    • Because it can't provide authoritative suggestions, I think this lint (if it's created) should be optional.
    • I'm not familiar with the actual performance gains, and it will depend on the app, but this seems like a helpful reminder for large, enterprise-grade codebases where good local decision making could add up to significant performance improvements.
  • This lint could also suggest annotating class with @immutable. (consider_const_constructor lint would be similar to prefer_const_constructors_in_immutables).

Discussion checklist

  • List any existing rules this proposal modifies, complements, overlaps or conflicts with.
  • List any relevant issues (reported here, the SDK Tracker, or elsewhere).
  • If there's any prior art (e.g., in other linters), please add references here.
  • If this proposal corresponds to Effective Dart or Flutter Style Guide advice, please call it out. (If there isn't any corresponding advice, should there be?)
  • If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.
@a14n
Copy link
Contributor

a14n commented Jan 13, 2023

I'd rather go in this direction. I'd prefer an add_immutable_annotation lint that trigger on classes with only final fields. Thus there would be no overlapping with other lints and it could easily be used together with prefer_const_constructors_in_immutables.

@incendial
Copy link
Contributor

incendial commented Jan 19, 2023

@pq pq added the P3 A lower priority bug or feature request label May 22, 2023
@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Mar 27, 2024
@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 19, 2024
@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 19, 2024
@bwilkerson bwilkerson added area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. and removed legacy-area-analyzer Use area-devexp instead. labels Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-linter Issues with the analyzer's support for the linter package linter-lint-proposal linter-set-flutter linter-status-pending P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants