Skip to content

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

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
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

samuelkchris
Copy link

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:

  1. Specific Method Names: Error message now includes the exact method being used (e.g., "bool.hasEnvironment", "String.fromEnvironment")

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

  3. Actionable Guidance: Suggests using Platform.environment for runtime access instead

  4. Comprehensive Documentation:

    • Explains compile-time vs runtime behavior
    • Shows both BAD and GOOD examples
    • Provides concrete alternatives
  5. 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

Copy link

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).

@samuelkchris samuelkchris force-pushed the fix-do-not-use-environment-lint-message branch from 71af238 to 8fbffe0 Compare June 20, 2025 21:45
…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
@samuelkchris samuelkchris force-pushed the fix-do-not-use-environment-lint-message branch from f5c0df9 to bf3933c Compare June 20, 2025 21:56
Copy link

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

1 similar comment
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

do_not_use_environment lint message is very confusing ("Invalid use of a declared variable")
1 participant