merge/audit-and-state-map #7
5 changed files with 282 additions and 0 deletions
fix(check): lower confidence where unresolved code can hide evidence
check never looked at analyzer errors, so a project that resolved only partly was analysed as if it were whole. pipe reads names off the syntax tree, and an unresolved `Foo()` is a method call rather than a construction, so a dispatch, emission or construction in such a file simply disappears - and every finding is an absence claim that the missing evidence could refute. Found live during this work: with the Flutter SDK unreadable, nuwayze resolved with 40,079 analyzer errors (package:flutter unresolvable) and check still reported 102 findings at full confidence, with no warning. Now, when any analysed file has an unresolved-reference diagnostic (uri_does_not_exist, undefined_*, creation_with_non_type, ...): - check warns on stderr and adds a report note naming the first one; - a finding is lowered one confidence step, with data.confidenceLowered = unresolved-references, when an unresolved file mentions one of its identity names (event, variant, union, owner, provider, member). pipe's evidence is name-based, so a file that never names X cannot hide evidence about X; lowering everything for one stray error would bury the signal (a first cut did exactly that, taking all 102 nuwayze findings down for a single unresolved file). Validated on mirrors of the dogfood repos with a readable SDK (0 analyzer errors): nuwayze 102 findings identical to the original main, dex 0, word_alive 1 and tick 1 (the intended riverpod change). The fixture test adds an unresolved file that constructs NeverDispatched: its dead-handler finding drops to medium while UnhandledTap, which that file never names, stays high.
commit
17ff523a11
|
|
@ -100,6 +100,15 @@ the standalone SDK. If the project has never been resolved, `pipe` says so and
|
||||||
tells you to run `pub get` rather than silently degrading every `package:` import
|
tells you to run `pub get` rather than silently degrading every `package:` import
|
||||||
to `dynamic`.
|
to `dynamic`.
|
||||||
|
|
||||||
|
A project that resolves only partly is reported, not silently analysed: an
|
||||||
|
unresolved `Foo()` is a method call rather than a construction, so evidence in
|
||||||
|
that file is invisible. `check` warns on stderr, notes how many files have
|
||||||
|
unresolved references, and lowers by one step the confidence of every finding
|
||||||
|
whose names such a file mentions (`data.confidenceLowered`). An SDK that exists
|
||||||
|
but cannot be read (for example a Flutter SDK under a privacy-protected macOS
|
||||||
|
folder, which makes `package:flutter` unresolvable) is refused up front with a
|
||||||
|
`--sdk` remedy.
|
||||||
|
|
||||||
## CI and ratcheting
|
## CI and ratcheting
|
||||||
|
|
||||||
An existing codebase will have findings on day one. Accept them once, then fail
|
An existing codebase will have findings on day one. Accept them once, then fail
|
||||||
|
|
|
||||||
|
|
@ -4,6 +4,8 @@ import 'package:analyzer/dart/analysis/analysis_context.dart';
|
||||||
import 'package:analyzer/dart/analysis/analysis_context_collection.dart';
|
import 'package:analyzer/dart/analysis/analysis_context_collection.dart';
|
||||||
import 'package:analyzer/dart/analysis/results.dart';
|
import 'package:analyzer/dart/analysis/results.dart';
|
||||||
import 'package:analyzer/dart/analysis/session.dart';
|
import 'package:analyzer/dart/analysis/session.dart';
|
||||||
|
import 'package:analyzer/dart/ast/ast.dart';
|
||||||
|
import 'package:analyzer/dart/ast/visitor.dart';
|
||||||
import 'package:analyzer/diagnostic/diagnostic.dart';
|
import 'package:analyzer/diagnostic/diagnostic.dart';
|
||||||
import 'package:analyzer/file_system/physical_file_system.dart';
|
import 'package:analyzer/file_system/physical_file_system.dart';
|
||||||
|
|
||||||
|
|
@ -35,6 +37,100 @@ class FileOutcome {
|
||||||
List<Diagnostic> get diagnostics => unit?.diagnostics ?? const <Diagnostic>[];
|
List<Diagnostic> get diagnostics => unit?.diagnostics ?? const <Diagnostic>[];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Diagnostic codes meaning a name or import did not resolve. Unlike a type
|
||||||
|
/// error, these change the syntax tree pipe reads: an unresolved `Foo()` is a
|
||||||
|
/// method call rather than a construction, so a dispatch or emission of `Foo`
|
||||||
|
/// disappears from the analysis.
|
||||||
|
const Set<String> unresolvedReferenceCodes = {
|
||||||
|
'uri_does_not_exist',
|
||||||
|
'undefined_class',
|
||||||
|
'undefined_identifier',
|
||||||
|
'undefined_method',
|
||||||
|
'undefined_function',
|
||||||
|
'undefined_getter',
|
||||||
|
'undefined_setter',
|
||||||
|
'undefined_super_member',
|
||||||
|
'undefined_prefixed_name',
|
||||||
|
'creation_with_non_type',
|
||||||
|
'extends_non_class',
|
||||||
|
'implements_non_class',
|
||||||
|
'mixin_of_non_class',
|
||||||
|
'non_type_as_type_argument',
|
||||||
|
};
|
||||||
|
|
||||||
|
/// Resolved files that contain at least one unresolved reference.
|
||||||
|
class UnresolvedReferences {
|
||||||
|
UnresolvedReferences({
|
||||||
|
required this.files,
|
||||||
|
required this.analysed,
|
||||||
|
this.first,
|
||||||
|
this.names = const {},
|
||||||
|
});
|
||||||
|
|
||||||
|
/// Every identifier those files mention. pipe's evidence is name-based, so an
|
||||||
|
/// unresolved file can only hide evidence about a name it mentions.
|
||||||
|
final Set<String> names;
|
||||||
|
|
||||||
|
/// Paths (absolute) of resolved files with an unresolved reference.
|
||||||
|
final List<String> files;
|
||||||
|
|
||||||
|
/// How many files resolved at all.
|
||||||
|
final int analysed;
|
||||||
|
|
||||||
|
/// The first such diagnostic as `path:line: message`, for the reader.
|
||||||
|
final String? first;
|
||||||
|
|
||||||
|
bool get any => files.isNotEmpty;
|
||||||
|
}
|
||||||
|
|
||||||
|
UnresolvedReferences findUnresolvedReferences(List<FileOutcome> outcomes) {
|
||||||
|
final files = <String>[];
|
||||||
|
final names = <String>{};
|
||||||
|
String? first;
|
||||||
|
var analysed = 0;
|
||||||
|
for (final outcome in outcomes) {
|
||||||
|
final unit = outcome.unit;
|
||||||
|
if (unit == null) continue;
|
||||||
|
analysed++;
|
||||||
|
for (final diagnostic in unit.diagnostics) {
|
||||||
|
if (!unresolvedReferenceCodes.contains(
|
||||||
|
diagnostic.diagnosticCode.lowerCaseName,
|
||||||
|
)) {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
files.add(outcome.path);
|
||||||
|
final line = unit.lineInfo.getLocation(diagnostic.offset).lineNumber;
|
||||||
|
first ??= '${outcome.path}:$line: ${diagnostic.message}';
|
||||||
|
final collector = _Names();
|
||||||
|
unit.unit.accept(collector);
|
||||||
|
names.addAll(collector.names);
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return UnresolvedReferences(
|
||||||
|
files: files,
|
||||||
|
analysed: analysed,
|
||||||
|
first: first,
|
||||||
|
names: names,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
class _Names extends RecursiveAstVisitor<void> {
|
||||||
|
final Set<String> names = {};
|
||||||
|
|
||||||
|
@override
|
||||||
|
void visitSimpleIdentifier(SimpleIdentifier node) {
|
||||||
|
names.add(node.name);
|
||||||
|
super.visitSimpleIdentifier(node);
|
||||||
|
}
|
||||||
|
|
||||||
|
@override
|
||||||
|
void visitNamedType(NamedType node) {
|
||||||
|
names.add(node.name.lexeme);
|
||||||
|
super.visitNamedType(node);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Owns the analyzer contexts for one target project.
|
/// Owns the analyzer contexts for one target project.
|
||||||
///
|
///
|
||||||
/// Mirrors the lifecycle Flutter's own tooling uses: one collection rooted at
|
/// Mirrors the lifecycle Flutter's own tooling uses: one collection rooted at
|
||||||
|
|
|
||||||
|
|
@ -1,3 +1,6 @@
|
||||||
|
import 'dart:io';
|
||||||
|
|
||||||
|
import 'analysis.dart';
|
||||||
import 'index.dart';
|
import 'index.dart';
|
||||||
import 'project.dart';
|
import 'project.dart';
|
||||||
import 'rules.dart';
|
import 'rules.dart';
|
||||||
|
|
@ -42,6 +45,15 @@ Future<ProjectAnalysis> analyzeProject(
|
||||||
);
|
);
|
||||||
try {
|
try {
|
||||||
final flow = buildStateFlow(project, run.outcomes);
|
final flow = buildStateFlow(project, run.outcomes);
|
||||||
|
final unresolved = findUnresolvedReferences(run.outcomes);
|
||||||
|
if (unresolved.any) {
|
||||||
|
stderr.writeln(
|
||||||
|
'pipe: WARNING ${unresolved.files.length} of ${unresolved.analysed} '
|
||||||
|
'analysed file(s) have unresolved references, so findings naming '
|
||||||
|
'anything they mention may be false and have lowered confidence. Run '
|
||||||
|
'`pipe index` to see why.',
|
||||||
|
);
|
||||||
|
}
|
||||||
final report = runChecks(
|
final report = runChecks(
|
||||||
project: project,
|
project: project,
|
||||||
outcomes: run.outcomes,
|
outcomes: run.outcomes,
|
||||||
|
|
|
||||||
|
|
@ -129,6 +129,23 @@ class Finding {
|
||||||
return file ?? fingerprint;
|
return file ?? fingerprint;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// This finding one confidence step lower, recording why.
|
||||||
|
Finding lowered(String reason) => Finding(
|
||||||
|
rule: rule,
|
||||||
|
severity: severity,
|
||||||
|
confidence: switch (confidence) {
|
||||||
|
'high' => 'medium',
|
||||||
|
_ => 'low',
|
||||||
|
},
|
||||||
|
message: message,
|
||||||
|
file: file,
|
||||||
|
line: line,
|
||||||
|
evidence: evidence,
|
||||||
|
remediation: remediation,
|
||||||
|
data: {...data, 'confidenceLowered': reason},
|
||||||
|
baselined: baselined,
|
||||||
|
);
|
||||||
|
|
||||||
Finding asBaselined() => Finding(
|
Finding asBaselined() => Finding(
|
||||||
rule: rule,
|
rule: rule,
|
||||||
severity: severity,
|
severity: severity,
|
||||||
|
|
@ -821,6 +838,34 @@ FindingReport runChecks({
|
||||||
if (note.isNotEmpty) notes.add(note);
|
if (note.isNotEmpty) notes.add(note);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Every finding is a whole-program absence claim (never rendered, never
|
||||||
|
// dispatched, never notified), and an unresolved reference can hide the
|
||||||
|
// evidence that refutes it: an unresolved `Foo()` is a method call, not a
|
||||||
|
// construction. On a real app whose package:flutter could not be read,
|
||||||
|
// findings were reported at high confidence with no indication. pipe's
|
||||||
|
// evidence is name-based, so only a finding whose names an unresolved file
|
||||||
|
// mentions can have lost evidence there; lowering every finding for one
|
||||||
|
// stray error elsewhere would bury the signal.
|
||||||
|
final unresolved = findUnresolvedReferences(outcomes);
|
||||||
|
if (unresolved.any) {
|
||||||
|
var lowered = 0;
|
||||||
|
for (var i = 0; i < findings.length; i++) {
|
||||||
|
if (!_identityNames(findings[i]).any(unresolved.names.contains)) continue;
|
||||||
|
findings[i] = findings[i].lowered('unresolved-references');
|
||||||
|
lowered++;
|
||||||
|
}
|
||||||
|
final first = unresolved.first!;
|
||||||
|
notes.add(
|
||||||
|
'${unresolved.files.length} of ${unresolved.analysed} analysed file(s) '
|
||||||
|
'have unresolved references (first: '
|
||||||
|
'${first.replaceFirst('${project.root}/', '')}). An unresolved `Foo()` '
|
||||||
|
'is not seen as a construction, so evidence in those files can be '
|
||||||
|
'missed: $lowered finding(s) naming something they mention had their '
|
||||||
|
'confidence lowered one step. Run `pipe index` and resolve the project '
|
||||||
|
'(`pub get`, a readable SDK) for full confidence.',
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
final resolved = baseline == null
|
final resolved = baseline == null
|
||||||
? findings
|
? findings
|
||||||
: [
|
: [
|
||||||
|
|
@ -1022,6 +1067,31 @@ class _CalledNames extends RecursiveAstVisitor<void> {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The names a finding makes a claim about: the values of its identity keys.
|
||||||
|
Set<String> _identityNames(Finding finding) {
|
||||||
|
const keys = [
|
||||||
|
'union',
|
||||||
|
'variant',
|
||||||
|
'variants',
|
||||||
|
'group',
|
||||||
|
'event',
|
||||||
|
'provider',
|
||||||
|
'owner',
|
||||||
|
'member',
|
||||||
|
'members',
|
||||||
|
'stateType',
|
||||||
|
'valueType',
|
||||||
|
'valueTypes',
|
||||||
|
];
|
||||||
|
final names = <String>{};
|
||||||
|
for (final key in keys) {
|
||||||
|
final value = finding.data[key];
|
||||||
|
if (value is String) names.add(value);
|
||||||
|
if (value is List) names.addAll(value.whereType<String>());
|
||||||
|
}
|
||||||
|
return names;
|
||||||
|
}
|
||||||
|
|
||||||
/// `on<E>` for a bloc handler closure, so two handlers in one class get
|
/// `on<E>` for a bloc handler closure, so two handlers in one class get
|
||||||
/// distinct identities. As a bare `<closure>` they shared a fingerprint, and
|
/// distinct identities. As a bare `<closure>` they shared a fingerprint, and
|
||||||
/// the baseline dropped all but one of them.
|
/// the baseline dropped all but one of them.
|
||||||
|
|
|
||||||
95
test/unresolved_test.dart
Normal file
95
test/unresolved_test.dart
Normal file
|
|
@ -0,0 +1,95 @@
|
||||||
|
import 'dart:io';
|
||||||
|
|
||||||
|
import 'package:pipe/pipe.dart';
|
||||||
|
import 'package:test/test.dart';
|
||||||
|
|
||||||
|
/// The fixture plus one file whose references do not resolve: it constructs
|
||||||
|
/// `NeverDispatched` without importing it, and imports a file that is missing.
|
||||||
|
String _corpusWithUnresolvedFile() {
|
||||||
|
final from = Directory('test/fixtures/stuck_loading');
|
||||||
|
final to = Directory.systemTemp.createTempSync('pipe-unresolved');
|
||||||
|
for (final entity in from.listSync(recursive: true)) {
|
||||||
|
final relative = entity.path.substring(from.path.length + 1);
|
||||||
|
final target = '${to.path}/$relative';
|
||||||
|
if (entity is Directory) {
|
||||||
|
Directory(target).createSync(recursive: true);
|
||||||
|
} else if (entity is File) {
|
||||||
|
File(target)
|
||||||
|
..createSync(recursive: true)
|
||||||
|
..writeAsBytesSync(entity.readAsBytesSync());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
File('${to.path}/lib/unresolved_ui.dart').writeAsStringSync('''
|
||||||
|
import 'widgets_that_do_not_exist.dart';
|
||||||
|
|
||||||
|
// Unresolved, `NeverDispatched()` parses as a method call rather than a
|
||||||
|
// construction, so the dispatch below is invisible to the analysis.
|
||||||
|
void tapNever(dynamic bloc) => bloc.add(NeverDispatched());
|
||||||
|
''');
|
||||||
|
return to.path;
|
||||||
|
}
|
||||||
|
|
||||||
|
void main() {
|
||||||
|
test(
|
||||||
|
'a finding an unresolved file could refute is not high confidence',
|
||||||
|
() async {
|
||||||
|
// Every pipe finding is a whole-program absence claim, and an unresolved
|
||||||
|
// reference anywhere can hide the evidence that refutes it. On a real app
|
||||||
|
// whose package:flutter could not be read (40k analyzer errors), check
|
||||||
|
// reported findings at high confidence with no warning at all.
|
||||||
|
final analysis = await analyzeProject(
|
||||||
|
await loadProject(_corpusWithUnresolvedFile()),
|
||||||
|
concurrency: 1,
|
||||||
|
);
|
||||||
|
try {
|
||||||
|
final findings = analysis.report.findings;
|
||||||
|
final dead = findings.singleWhere(
|
||||||
|
(f) =>
|
||||||
|
f.rule == ruleDeadHandler && f.data['event'] == 'NeverDispatched',
|
||||||
|
);
|
||||||
|
expect(dead.confidence, 'medium');
|
||||||
|
expect(dead.data['confidenceLowered'], 'unresolved-references');
|
||||||
|
// The unresolved file never mentions UnhandledTap, so it cannot hide
|
||||||
|
// evidence about it: one stray error must not bury every finding.
|
||||||
|
final untouched = findings.singleWhere(
|
||||||
|
(f) =>
|
||||||
|
f.rule == ruleEventWithoutHandler &&
|
||||||
|
f.data['event'] == 'UnhandledTap',
|
||||||
|
);
|
||||||
|
expect(untouched.confidence, 'high');
|
||||||
|
expect(untouched.data.containsKey('confidenceLowered'), isFalse);
|
||||||
|
expect(
|
||||||
|
analysis.report.notes.join('\n'),
|
||||||
|
allOf(
|
||||||
|
contains('unresolved references'),
|
||||||
|
contains('unresolved_ui.dart'),
|
||||||
|
contains('1 finding(s)'),
|
||||||
|
),
|
||||||
|
);
|
||||||
|
} finally {
|
||||||
|
await analysis.dispose();
|
||||||
|
}
|
||||||
|
},
|
||||||
|
);
|
||||||
|
|
||||||
|
test('a fully resolved project keeps its confidence', () async {
|
||||||
|
final analysis = await analyzeProject(
|
||||||
|
await loadProject('test/fixtures/stuck_loading'),
|
||||||
|
concurrency: 1,
|
||||||
|
);
|
||||||
|
try {
|
||||||
|
expect(
|
||||||
|
analysis.report.findings.where((f) => f.confidence == 'high'),
|
||||||
|
isNotEmpty,
|
||||||
|
);
|
||||||
|
expect(
|
||||||
|
analysis.report.findings.where(
|
||||||
|
(f) => f.data.containsKey('confidenceLowered'),
|
||||||
|
),
|
||||||
|
isEmpty,
|
||||||
|
);
|
||||||
|
} finally {
|
||||||
|
await analysis.dispose();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
}
|
||||||
Loading…
Add table
Add a link
Reference in a new issue