Skip to content
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

Validate part directive for libs with parts #576

Merged
Show file tree
Hide file tree
Changes from 2 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: 1 addition & 4 deletions lib/src/builder/builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,7 @@ class OverReactBuilder extends Builder {
}

// Collect all of the part files for this library.
final parts = libraryUnit.directives
.whereType<PartDirective>()
// Ignore all generated `.g.dart` parts.
.where((part) => !part.uri.stringValue.endsWith('.g.dart'));
final parts = getNonGeneratedParts(libraryUnit);

// Generate over_react code for the input library.
generateForFile(source, buildStep.inputId, libraryUnit);
Expand Down
6 changes: 6 additions & 0 deletions lib/src/builder/util.dart
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ Uri idToPackageUri(AssetId id) {
path: p.url.join(id.package, id.path.replaceFirst('lib/', '')));
}

Iterable<PartDirective> getNonGeneratedParts(CompilationUnit libraryUnit) {
greglittlefield-wf marked this conversation as resolved.
Show resolved Hide resolved
return libraryUnit.directives
.whereType<PartDirective>()
// Ignore all generated `.g.dart` parts.
.where((part) => !part.uri.stringValue.endsWith('.g.dart'));
}
/// Returns true if the given compilation unit is a part file.
bool isPart(CompilationUnit unit) =>
unit.directives.any((directive) => directive is PartOfDirective);
Expand Down
81 changes: 71 additions & 10 deletions tools/analyzer_plugin/lib/src/diagnostic/boilerplate_validator.dart
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import 'dart:async';
import 'dart:io';

import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/dart/analysis/utilities.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/source/source_range.dart';
import 'package:analyzer_plugin/protocol/protocol_common.dart';
import 'package:meta/meta.dart';
// ignore: implementation_imports
import 'package:over_react/src/builder/parsing.dart';
// ignore: implementation_imports
import 'package:over_react/src/builder/util.dart';
import 'package:over_react_analyzer_plugin/src/diagnostic_contributor.dart';
import 'package:over_react_analyzer_plugin/src/util/boilerplate_utils.dart';
import 'package:source_span/source_span.dart';
Expand All @@ -31,12 +35,28 @@ class BoilerplateValidatorDiagnostic extends DiagnosticContributor {
PartDirective _overReactGeneratedPartDirective;
bool _overReactGeneratedPartDirectiveIsValid;

@override
Future<void> computeErrors(result, collector) async {
_overReactGeneratedPartDirective = getOverReactGeneratedPartDirective(result.unit);
_overReactGeneratedPartDirectiveIsValid = _overReactGeneratedPartDirective != null &&
overReactGeneratedPartDirectiveIsValid(_overReactGeneratedPartDirective, result.uri);
/// Returns true if the [unit] representing a part file has declarations.
///
/// Does not report any errors for the part file, as those are handled when the part file is analyzed
bool _partHasDeclarations(CompilationUnit unit, ResolvedUnitResult parentResult) {
return getBoilerplateDeclarations(detectBoilerplateMembers(unit), ErrorCollector.callback(
SourceFile.fromString(parentResult.content, url: parentResult.path),
onError: (message, [span]) {
// no-op. It is assumed this method will run for parent files, and the part file will get analyzed in it's own
// context
},
onWarning: (message, [span]) {
// no-op. It is assumed this method will run for parent files, and the part file will get analyzed in it's own
// context
},
)).isNotEmpty;
}

/// Computes errors for over_react boilerplate
///
/// Also returns whether the component has valid over_react declarations, which is useful in determining whether to
/// validate the generated part directive.
Future<bool> _computeBoilerPlateErrors(ResolvedUnitResult result, DiagnosticCollector collector) async {
greglittlefield-wf marked this conversation as resolved.
Show resolved Hide resolved
final debugMatch = _debugFlagPattern.firstMatch(result.content);
final debug = debugMatch != null;
if (debug) {
Expand Down Expand Up @@ -67,6 +87,8 @@ class BoilerplateValidatorDiagnostic extends DiagnosticContributor {
// Do not lint anything that is not a likely boilerplate member that will actually get generated.
if (member.versionConfidences.toList().every((vcp) => vcp.confidence <= Confidence.neutral)) continue;

if (isPart(result.unit)) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of this, there's no error if components are declared in parts and there's no generated import in the main library.

Sorry, I can't remember; we going to handle that case in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

For posterity: we'll handle this separately


if (_overReactGeneratedPartDirective == null) {
await _addPartDirectiveErrorForMember(
result: result,
Expand All @@ -82,6 +104,7 @@ class BoilerplateValidatorDiagnostic extends DiagnosticContributor {
errorType: PartDirectiveErrorType.invalid,
);
}

}

final declarations = getBoilerplateDeclarations(members, errorCollector).toList();
Expand All @@ -100,10 +123,11 @@ class BoilerplateValidatorDiagnostic extends DiagnosticContributor {
}
}
}
return declarations.isNotEmpty;
}

// TODO: this logic does not handle when the component is already in a part file. We should refactor this to share logic that the builder uses to validate parts.
// <https://github.com/Workiva/over_react/issues/522>
if (declarations.isEmpty && _overReactGeneratedPartDirective != null) {
Future<void> _computePartDirectiveErrors(ResolvedUnitResult result, DiagnosticCollector collector, bool hasDeclarations) async {
if (!hasDeclarations && _overReactGeneratedPartDirective != null) {
await collector.addErrorWithFix(
errorCode,
result.locationFor(_overReactGeneratedPartDirective),
Expand All @@ -118,13 +142,13 @@ class BoilerplateValidatorDiagnostic extends DiagnosticContributor {
removeOverReactGeneratedPartDirective(builder, result.unit);
}),
);
} else if (declarations.isNotEmpty &&
} else if (hasDeclarations &&
_overReactGeneratedPartDirective != null &&
!_overReactGeneratedPartDirectiveIsValid) {
await collector.addErrorWithFix(
errorCode,
result.locationFor(_overReactGeneratedPartDirective),
errorMessageArgs: ['The generated part name must match the name of the file that contains it.'],
errorMessageArgs: ['The generated part name must match the name of the file that contains it, but with `over_react.g.dart` for the file extension.'],
fixKind: invalidGeneratedPartFixKind,
computeFix: () => buildFileEdit(result, (builder) {
fixOverReactGeneratedPartDirective(builder, result.unit, result.uri);
Expand All @@ -133,6 +157,43 @@ class BoilerplateValidatorDiagnostic extends DiagnosticContributor {
}
}

@override
Future<void> computeErrors(result, collector) async {
_overReactGeneratedPartDirective = getOverReactGeneratedPartDirective(result.unit);
_overReactGeneratedPartDirectiveIsValid =_overReactGeneratedPartDirective != null &&
overReactGeneratedPartDirectiveIsValid(_overReactGeneratedPartDirective, result.uri);

final hasDeclarations = await _computeBoilerPlateErrors(result, collector);
if (isPart(result.unit)) {
return;
}

final parts = getNonGeneratedParts(result.unit);

// compute errors for parts files
var anyPartHasDeclarations = false;
for (final part in parts) {
final uri = Uri.parse(part.uriElement.toString());
final fileContents = File.fromUri(uri).readAsStringSync();

final partResult = parseString(content: fileContents,
path: part.uriElement.toString(),
throwIfDiagnostics: false);

if (partResult.errors.isNotEmpty) {
// Error parsing file, skip.
continue;
}

greglittlefield-wf marked this conversation as resolved.
Show resolved Hide resolved
if (_partHasDeclarations(partResult.unit, result)) {
anyPartHasDeclarations = true;
}
}

await _computePartDirectiveErrors(
result, collector, hasDeclarations || anyPartHasDeclarations);
}

Future<void> _addPartDirectiveErrorForMember({
@required ResolvedUnitResult result,
@required DiagnosticCollector collector,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
library invalid_generated_part_filename;

import 'package:over_react/over_react.dart';
part 'invalid_gnerated_part_filename_parent.over_react.g.dart';

part 'invalid_generated_part_filename_part.dart';
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//part 'invaid_generated_part_filename_part.over_react.g.dart';
part of invalid_generated_part_filename;

UiFactory<FooProps> Foo = _$Foo; // ignore: undefined_identifier

mixin FooProps on UiProps {}

class FooComponent extends UiComponent2<FooProps> {
@override
render() {}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import 'package:over_react/over_react.dart';

part 'invalid_generated_part_filename.over_react.g.dart';
part 'invalid_generated_pat_filename_standalone.over_react.g.dart';

UiFactory<FooProps> Foo = _$Foo; // ignore: undefined_identifier

Expand Down