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 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions pkg/linter/lib/src/lint_codes.g.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

correctionMessage:
"Try using 'Platform.environment' for runtime access or remove environment-dependent code.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);

static const LintCode document_ignores = LinterLintCode(
Expand Down
19 changes: 18 additions & 1 deletion pkg/linter/lib/src/rules/do_not_use_environment.dart
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,24 @@ class _Visitor extends SimpleAstVisitor<void> {
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 {
throw StateError(
'Unexpected type for environment constructor: $staticType',
);
}
String fullMethodName = '$typeName.$constructorName';
rule.reportAtNode(
node,
arguments: [fullMethodName],
diagnosticCode: rule.diagnosticCode,
);
}
}

Expand Down
20 changes: 15 additions & 5 deletions pkg/linter/messages.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4149,22 +4149,32 @@ LintCode:
Future<void> 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


**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:**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

```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."
Expand Down
20 changes: 20 additions & 0 deletions pkg/linter/test/rules/do_not_use_environment_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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)],
);
}
}