From bf3933c7ce023a75b5738f5890c8c34b4156e18f Mon Sep 17 00:00:00 2001 From: Samuel Ssekizinvu Date: Sat, 21 Jun 2025 00:56:18 +0300 Subject: [PATCH 1/2] [linter] Improve do_not_use_environment lint error message and documentation 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 https://github.com/dart-lang/sdk/issues/59203 --- pkg/linter/lib/src/lint_codes.g.dart | 5 +++-- .../lib/src/rules/do_not_use_environment.dart | 17 +++++++++++++++- pkg/linter/messages.yaml | 20 ++++++++++++++----- .../rules/do_not_use_environment_test.dart | 20 +++++++++++++++++++ 4 files changed, 54 insertions(+), 8 deletions(-) diff --git a/pkg/linter/lib/src/lint_codes.g.dart b/pkg/linter/lib/src/lint_codes.g.dart index aa18d8eadf72..9d0436ba2dd2 100644 --- a/pkg/linter/lib/src/lint_codes.g.dart +++ b/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.", + correctionMessage: + "Try using 'Platform.environment' for runtime access or remove environment-dependent code.", ); static const LintCode document_ignores = LinterLintCode( diff --git a/pkg/linter/lib/src/rules/do_not_use_environment.dart b/pkg/linter/lib/src/rules/do_not_use_environment.dart index 5d843175904c..37e0b3243b2c 100644 --- a/pkg/linter/lib/src/rules/do_not_use_environment.dart +++ b/pkg/linter/lib/src/rules/do_not_use_environment.dart @@ -42,7 +42,22 @@ class _Visitor extends SimpleAstVisitor { staticType.isDartCoreString) && constructorName == 'fromEnvironment') || (staticType.isDartCoreBool && constructorName == 'hasEnvironment')) { - rule.reportAtNode(node); + String typeName; + if (staticType.isDartCoreBool) { + typeName = 'bool'; + } else if (staticType.isDartCoreInt) { + typeName = 'int'; + } else if (staticType.isDartCoreString) { + typeName = 'String'; + } else { + typeName = 'unknown'; + } + String fullMethodName = '$typeName.$constructorName'; + rule.reportAtNode( + node, + arguments: [fullMethodName], + diagnosticCode: rule.diagnosticCode, + ); } } diff --git a/pkg/linter/messages.yaml b/pkg/linter/messages.yaml index aa426f639cc6..3466ca7b093f 100644 --- a/pkg/linter/messages.yaml +++ b/pkg/linter/messages.yaml @@ -4149,22 +4149,32 @@ LintCode: Future createDir(String path) async {} ``` do_not_use_environment: - problemMessage: "Invalid use of an environment declaration." - correctionMessage: "Try removing the environment declaration usage." + problemMessage: "Avoid using environment values like '{0}' which create hidden global state." + correctionMessage: "Try using 'Platform.environment' for runtime access or remove environment-dependent code." 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 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. **DON'T** use `fromEnvironment` or `hasEnvironment` factory constructors. **BAD:** ```dart - const loggingLevel = - bool.hasEnvironment('logging') ? String.fromEnvironment('logging') : null; + const bool usingAppEngine = bool.hasEnvironment('APPENGINE_RUNTIME'); + const loggingLevel = String.fromEnvironment('LOGGING_LEVEL'); + ``` + + **GOOD:** + ```dart + import 'dart:io'; + + final bool usingAppEngine = Platform.environment.containsKey('APPENGINE_RUNTIME'); + final String loggingLevel = Platform.environment['LOGGING_LEVEL'] ?? 'info'; ``` document_ignores: problemMessage: "Missing documentation explaining why the diagnostic is ignored." diff --git a/pkg/linter/test/rules/do_not_use_environment_test.dart b/pkg/linter/test/rules/do_not_use_environment_test.dart index 91126b753818..acd9fb4906da 100644 --- a/pkg/linter/test/rules/do_not_use_environment_test.dart +++ b/pkg/linter/test/rules/do_not_use_environment_test.dart @@ -81,4 +81,24 @@ void f() { [lint(13, 22)], ); } + + test_messageFormatting() async { + await assertDiagnostics( + r''' +void f() { + bool.fromEnvironment('DEBUG'); +} +''', + [lint(13, 27, messageContains: 'bool.fromEnvironment')], + ); + } + + test_constContext() async { + await assertDiagnostics( + r''' +const bool usingAppEngine = bool.hasEnvironment('APPENGINE_RUNTIME'); +''', + [lint(28, 19)], + ); + } } From cddf8c7758cf5c3bb2c5be4e43ec13f4e00ac3b2 Mon Sep 17 00:00:00 2001 From: Samuel Ssekizinvu Date: Thu, 26 Jun 2025 10:32:56 +0300 Subject: [PATCH 2/2] Replace 'unknown' with StateError for unreachable case Per code review feedback, using a throw statement makes it clear to readers that this case cannot be reached under normal circumstances. --- pkg/linter/lib/src/rules/do_not_use_environment.dart | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/linter/lib/src/rules/do_not_use_environment.dart b/pkg/linter/lib/src/rules/do_not_use_environment.dart index 37e0b3243b2c..50ac6f6dcd39 100644 --- a/pkg/linter/lib/src/rules/do_not_use_environment.dart +++ b/pkg/linter/lib/src/rules/do_not_use_environment.dart @@ -50,7 +50,9 @@ class _Visitor extends SimpleAstVisitor { } else if (staticType.isDartCoreString) { typeName = 'String'; } else { - typeName = 'unknown'; + throw StateError( + 'Unexpected type for environment constructor: $staticType', + ); } String fullMethodName = '$typeName.$constructorName'; rule.reportAtNode(