[linter] Improve do_not_use_environment lint error message and documentation #60970

samuelkchris · 2025-06-20T21:05:06Z

Summary

Improves the do_not_use_environment lint rule error message and documentation to be more helpful and actionable.

Problem

The current error message is confusing and unhelpful:

Before: "Invalid use of an environment declaration."
Users don't understand WHY they shouldn't use environment constructors
No guidance on what to use instead
The term "environment declaration" is unclear

Solution

After: "Avoid using environment values like 'bool.hasEnvironment' which create hidden global state."

Key Improvements:

Specific Method Names: Error message now includes the exact method being used (e.g., "bool.hasEnvironment", "String.fromEnvironment")
Clear Explanation: Explains that environment constructors create "hidden global state" which makes code hard to understand and maintain
Actionable Guidance: Suggests using Platform.environment for runtime access instead
Comprehensive Documentation:
- Explains compile-time vs runtime behavior
- Shows both BAD and GOOD examples
- Provides concrete alternatives
Better Test Coverage: Added tests for all variants and edge cases

Examples

Before

Invalid use of an environment declaration.
Try removing the environment declaration usage.

After

Avoid using environment values like 'bool.hasEnvironment' which create hidden global state.
Try using 'Platform.environment' for runtime access or remove environment-dependent code.

Documentation Improvements

Added clear examples in the lint documentation:

BAD:

const bool usingAppEngine = bool.hasEnvironment('APPENGINE_RUNTIME');
const loggingLevel = String.fromEnvironment('LOGGING_LEVEL');

GOOD:

import 'dart:io';

// Runtime access to environment variables
final bool usingAppEngine = Platform.environment.containsKey('APPENGINE_RUNTIME');
final String loggingLevel = Platform.environment['LOGGING_LEVEL'] ?? 'info';

Files Changed

pkg/linter/messages.yaml - Updated message and documentation
pkg/linter/lib/src/rules/do_not_use_environment.dart - Pass method name to error
pkg/linter/lib/src/lint_codes.g.dart - Generated code update
pkg/linter/test/rules/do_not_use_environment_test.dart - Enhanced test coverage

Test Plan

✅ All existing tests pass
✅ Added comprehensive test coverage for message formatting
✅ Tested with the original issue example (bool.hasEnvironment('APPENGINE_RUNTIME'))
✅ Verified error messages include specific method names

Fixes #59203

copybara-service · 2025-06-20T21:07:58Z

Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at:

https://dart-review.googlesource.com/c/sdk/+/435744

Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly.

Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR).

…ntation Replaces confusing "Invalid use of an environment declaration" message with clear, actionable guidance that explains WHY environment constructors should be avoided and WHAT to use instead. Changes: - Problem message: Now includes specific method name and explains it creates "hidden global state" - Correction message: Suggests Platform.environment for runtime access - Documentation: Added comprehensive examples showing both bad and good patterns - Implementation: Pass method name to error message for better specificity - Tests: Fixed test to properly validate message contains specific method name Before: "Invalid use of an environment declaration." After: "Avoid using environment values like 'bool.fromEnvironment' which create hidden global state." The new message clearly explains: 1. WHAT is wrong (creates hidden global state) 2. WHY it's problematic (unpredictable across environments) 3. WHAT to do instead (use Platform.environment) Fixes dart-lang#59203

copybara-service · 2025-06-20T21:58:36Z

https://dart-review.googlesource.com/c/sdk/+/435744 has been updated with the latest commits from this pull request.

copybara-service · 2025-06-20T21:58:37Z

https://dart-review.googlesource.com/c/sdk/+/435744 has been updated with the latest commits from this pull request.

Per code review feedback, using a throw statement makes it clear to readers that this case cannot be reached under normal circumstances.

copybara-service · 2025-06-26T07:35:35Z

https://dart-review.googlesource.com/c/sdk/+/435744 has been updated with the latest commits from this pull request.

copybara-service · 2025-06-26T07:35:43Z

https://dart-review.googlesource.com/c/sdk/+/435744 has been updated with the latest commits from this pull request.

kevmoo · 2025-06-26T20:56:25Z

looks like you still have many failing tests.

samuelkchris · 2025-06-26T21:24:07Z

looks like you still have many failing tests.

i need to relook at my local development environment

lrhn

The changes here (but possibly the existing lint too, to some degree) feels like tey misunderstand what the fromEnvironment constructors do.

If the goal is to avoid using the compilation environment for anything, then just say that.

If the goal is to catch users who mistakenly think that .fromEnvironment reads the process environment, say that it doesn't.

But saying that Platform.environment['APPENGINE_RUNTIME'] is a GOOD replacement for String.fromEnvironment('APPENGINE_RUNTIME') is misleading and confusing.

lrhn · 2025-07-03T15:39:41Z

pkg/linter/lib/src/lint_codes.g.dart

@@ -623,8 +623,9 @@ class LinterLintCode extends LintCode {

  static const LintCode do_not_use_environment = LinterLintCode(
    LintNames.do_not_use_environment,
-    "Invalid use of an environment declaration.",
-    correctionMessage: "Try removing the environment declaration usage.",
+    "Avoid using environment values like '{0}' which create hidden global state.",


I don't understand what "hidden global state" means in this case.
Or how reading the compilation environment "create"s anything.

lrhn · 2025-07-03T15:39:42Z

pkg/linter/lib/src/lint_codes.g.dart

-    correctionMessage: "Try removing the environment declaration usage.",
+    "Avoid using environment values like '{0}' which create hidden global state.",
+    correctionMessage:
+        "Try using 'Platform.environment' for runtime access or remove environment-dependent code.",


This sounds misleading. Using Platform.environment is runtime access to something completely different.
Maybe it's what the user wanted, but it's not a just a switch from compile-time to runtime access.

lrhn · 2025-07-03T15:43:36Z

pkg/linter/messages.yaml

    state:
      stable: "2.9"
    categories: [errorProne]
    hasPublishedDocs: false
    deprecatedDetails: |-
-      Using values derived from the environment at compile-time, creates
+      Using values derived from environment variables at compile-time creates


This sound like those constructors derive values from the process environment at compile-time.
They do not. They access the compilation environment which is guaranteed to contain, fx,
bool.fromEnvironment('dart.library.core'). There is nothing wrong with using this, it's a recommended way of knowing whether a platform library is available.

So const String.fromEnvironment('APPENGINE_RUNTIME') will not find a process environment entry like export APPENGINE_RUNTIME=foobar, but it will reflect a dart -DAPPENGINE_RUNTIME=foobar passed to the compiler at compile time.

That's working as intended.

lrhn · 2025-07-03T15:44:28Z

pkg/linter/messages.yaml

+      const loggingLevel = String.fromEnvironment('LOGGING_LEVEL');
+      ```
+
+      **GOOD:**


This is not not a GOOD replacement, it is completely different.

Unless you expect that this was what the user wanted anyway, it's not a replacement for the bad code.

lrhn · 2025-07-03T15:45:22Z

pkg/linter/messages.yaml

      hidden global state and makes applications hard to understand and maintain.
+      Environment values are resolved at compile-time and become embedded in the
+      compiled code, making behavior unpredictable across different environments.


These constructors has nothing to do with "different environments" at all, they are, if anything, predictable.

lrhn · 2025-07-03T15:57:27Z

Clear Explanation: Explains that environment constructors create "hidden global state" which makes code hard to understand and maintain

Not really, I don't understand why that's a problem. Or how it creates hidden global state. Or what that even is.

The explanation has to be much more fundamental.

Either the user knows what String.fromEnvironment does, and you have to tell them why they shouldn't do that,
or they don't know, and you should explain what it does so they can decide whether it's what they want or not.

Consider:

The compilation environment is a configuration passed to Dart compilers set when a Dart program
is compiled, and const String.fromEnvironment('name') can access the value for the entry name in
that environment.

The compilation environment is not the same as the process environment, where variables set in
your shell can be seen. Use Platform.environment to access that.

There is rarely any reason for client code to read the compilation environment, any use is more likely
to be a mistake, so this lint disallows access to the compilation environment.
If you do have a use, use an // ignore: do_not_use_environment comment to silence the lint.
If you intended to access the process environment, see Platform.environment.

samuelkchris mentioned this pull request Jun 20, 2025

do_not_use_environment lint message is very confusing ("Invalid use of a declared variable") #59203

Open

samuelkchris force-pushed the fix-do-not-use-environment-lint-message branch from 71af238 to 8fbffe0 Compare June 20, 2025 21:45

samuelkchris force-pushed the fix-do-not-use-environment-lint-message branch from f5c0df9 to bf3933c Compare June 20, 2025 21:56

Replace 'unknown' with StateError for unreachable case

cddf8c7

Per code review feedback, using a throw statement makes it clear to readers that this case cannot be reached under normal circumstances.

lrhn added the area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. label Jul 3, 2025

lrhn reviewed Jul 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[linter] Improve do_not_use_environment lint error message and documentation #60970

[linter] Improve do_not_use_environment lint error message and documentation #60970

samuelkchris commented Jun 20, 2025

Uh oh!

copybara-service bot commented Jun 20, 2025

Uh oh!

copybara-service bot commented Jun 20, 2025

Uh oh!

copybara-service bot commented Jun 20, 2025

Uh oh!

copybara-service bot commented Jun 26, 2025

Uh oh!

copybara-service bot commented Jun 26, 2025

Uh oh!

kevmoo commented Jun 26, 2025

Uh oh!

samuelkchris commented Jun 26, 2025

Uh oh!

lrhn left a comment

Uh oh!

lrhn Jul 3, 2025

Uh oh!

lrhn Jul 3, 2025

Uh oh!

lrhn Jul 3, 2025

Uh oh!

lrhn Jul 3, 2025

Uh oh!

lrhn Jul 3, 2025

Uh oh!

lrhn commented Jul 3, 2025

Uh oh!

Uh oh!

[linter] Improve do_not_use_environment lint error message and documentation #60970

Are you sure you want to change the base?

[linter] Improve do_not_use_environment lint error message and documentation #60970

Conversation

samuelkchris commented Jun 20, 2025

Summary

Problem

Solution

Key Improvements:

Examples

Before

After

Documentation Improvements

Files Changed

Test Plan

Uh oh!

copybara-service bot commented Jun 20, 2025

Uh oh!

copybara-service bot commented Jun 20, 2025

Uh oh!

copybara-service bot commented Jun 20, 2025

Uh oh!

copybara-service bot commented Jun 26, 2025

Uh oh!

copybara-service bot commented Jun 26, 2025

Uh oh!

kevmoo commented Jun 26, 2025

Uh oh!

samuelkchris commented Jun 26, 2025

Uh oh!

lrhn left a comment

Choose a reason for hiding this comment

Uh oh!

lrhn Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

lrhn Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

lrhn Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

lrhn Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

lrhn Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

lrhn commented Jul 3, 2025

Uh oh!

Uh oh!