Skip to content

Commit

Permalink
Merge pull request #944 from Workiva/FED-2034-required-props-lint-res…
Browse files Browse the repository at this point in the history
…pect-forwarded

FED-2034: Analyzer plugin required props validation: check forwarded props
  • Loading branch information
rmconsole2-wf authored Sep 13, 2024
2 parents 6884cd8 + 5e37b83 commit 8f105ce
Show file tree
Hide file tree
Showing 11 changed files with 1,576 additions and 5 deletions.
2 changes: 1 addition & 1 deletion tools/analyzer_plugin/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ Currently-available debug info:
builder and analyzer functionality dealing with component declarations
- `// debug: over_react_metrics` - shows performance data on how long diagnostics took to run
- `// debug: over_react_exhaustive_deps` - shows info on how dependencies were detected/interpreted
- `// debug: over_react_required_props` - shows requiredness info for all props for each builder
- `// debug: over_react_required_props` - shows requiredness info for all props for each builder, as well as prop forwarding info

#### Attaching a Debugger
The dev experience when working on this plugin isn't ideal (See the `analyzer_plugin` debugging docs [for more information](https://github.com/dart-lang/sdk/blob/master/pkg/analyzer_plugin/doc/tutorial/debugging.md)), but it's possible debug and see logs from the plugin.
Expand Down
1 change: 1 addition & 0 deletions tools/analyzer_plugin/analysis_options.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ include: package:workiva_analysis_options/v1.recommended.yaml
analyzer:
exclude:
- playground/**
- test/temporary_test_fixtures/**
- test/test_fixtures/**
strong-mode:
implicit-casts: false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import 'package:over_react_analyzer_plugin/src/util/ast_util.dart';
import 'package:over_react_analyzer_plugin/src/util/null_safety_utils.dart';
import 'package:over_react_analyzer_plugin/src/util/pretty_print.dart';
import 'package:over_react_analyzer_plugin/src/util/prop_declarations/props_set_by_factory.dart';
import 'package:over_react_analyzer_plugin/src/util/prop_forwarding/forwarded_props.dart';
import 'package:over_react_analyzer_plugin/src/util/util.dart';
import 'package:over_react_analyzer_plugin/src/util/weak_map.dart';

Expand Down Expand Up @@ -67,6 +68,7 @@ class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor
"Missing required late prop {0}.",
AnalysisErrorSeverity.WARNING,
AnalysisErrorType.STATIC_WARNING,
correction: _correctionMessage,
);

// Note: this code is disabled by default in getDiagnosticContributors
Expand All @@ -76,8 +78,12 @@ class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor
"Missing @requiredProp {0}.",
AnalysisErrorSeverity.INFO,
AnalysisErrorType.STATIC_WARNING,
correction: _correctionMessage,
);

static const _correctionMessage =
"Either set this prop, or mix it into the enclosing component's props and forward it.";

static DiagnosticCode _codeForRequiredness(PropRequiredness requiredness) {
switch (requiredness) {
case PropRequiredness.late:
Expand Down Expand Up @@ -168,8 +174,9 @@ class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor
final debugHelper = AnalyzerDebugHelper(result, collector, enabled: _cachedIsDebugHelperEnabled(result));
// A flag to help verify during debugging/testing whether propsSetByFactory was computed.
var hasPropsSetByFactoryBeenComputed = false;
final debugSuppressedRequiredPropsDueToForwarding = <FieldElement>{};

// Use a late variable to compute this only when we need to.
// Use late variables to compute these only when we need to.
late final propsSetByFactory = () {
hasPropsSetByFactoryBeenComputed = true;

Expand All @@ -181,8 +188,8 @@ class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor

return _cachedGetPropsSetByFactory(factoryElement);
}();

final presentPropNames =
late final forwardedProps = computeForwardedProps(usage);
late final presentPropNames =
usage.cascadedProps.where((prop) => !prop.isPrefixed).map((prop) => prop.name.name).toSet();

for (final name in requiredPropInfo.requiredPropNames) {
Expand All @@ -198,7 +205,15 @@ class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor
continue;
}

// TODO(FED-2034) don't warn when we know required props are being forwarded
final sourcePropsClass = field.enclosingElement;
if (sourcePropsClass is InterfaceElement) {
if (forwardedProps != null && forwardedProps.definitelyForwardsPropsFrom(sourcePropsClass)) {
if (debugHelper.enabled) {
debugSuppressedRequiredPropsDueToForwarding.add(field);
}
continue;
}
}

// Only access propsSetByFactory when we hit missing required props to avoid computing it unnecessarily.
if (propsSetByFactory?.contains(name) ?? false) {
Expand All @@ -221,6 +236,20 @@ class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor
}
}

if (forwardedProps != null) {
debugHelper.log(() {
var message = StringBuffer()..writeln(forwardedProps);
if (debugSuppressedRequiredPropsDueToForwarding.isNotEmpty) {
final propsNamesByClassName = <String?, Set<String>>{};
for (final field in debugSuppressedRequiredPropsDueToForwarding) {
propsNamesByClassName.putIfAbsent(field.enclosingElement.name, () => {}).add(field.name);
}
message.write('Required props set only via forwarding: ${prettyPrint(propsNamesByClassName)}');
} else {}
return message.toString();
}, () => result.locationFor(forwardedProps.debugSourceNode));
}

// Include debug info for each invocation ahout all the props and their requirednesses.
debugHelper.log(() {
final propNamesByRequirednessName = <String, Set<String>>{};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/dart/element/type.dart';
import 'package:over_react_analyzer_plugin/src/diagnostic_contributor.dart';
import 'package:over_react_analyzer_plugin/src/util/ast_util.dart';
import 'package:over_react_analyzer_plugin/src/util/is_props_from_render.dart';
import 'package:over_react_analyzer_plugin/src/util/prop_forwarding/parse_forwarding_config.dart';
import 'package:over_react_analyzer_plugin/src/util/util.dart';

import 'forwarding_config.dart';
import 'util.dart';

/// A representation of props forwarded to a component usage.
class ForwardedProps {
/// The props class that props are being forwarded from.
///
/// For example, the type of `props` in `..addUnconsumedProps(props, ...)`.
final InterfaceElement propsClassBeingForwarded;

/// The configuration of which props to forward, or null if it could not be resolved.
final PropForwardingConfig? forwardingConfig;

/// A node that represents the addition of forwarded props, for use in debug infos only.
final AstNode debugSourceNode;

ForwardedProps(this.propsClassBeingForwarded, this.forwardingConfig, this.debugSourceNode);

/// Returns whether these forwarded props definitely include props from [propsClass], or false
/// if forwarded props could not be resolved.
///
/// This is true only when all of the following conditions are met:
/// - [propsClassBeingForwarded] inherits from [propsClass] (i.e., is or mixes in those props)
/// - [propsClass] is not excluded by [forwardingConfig]
bool definitelyForwardsPropsFrom(InterfaceElement propsClass) {
final forwardingConfig = this.forwardingConfig;
if (forwardingConfig == null) return false;

// Handle legacy classes being passed in.
if (propsClass.name.startsWith(r'_$')) {
// Look up the companion and use that instead, since that's what will be referenced in the forwarding config.
// E.g., for `_$FooProps`, find `FooProps`, since consumers will be using `FooProps` when setting up prop forwarding.
final companion = propsClassBeingForwarded.thisAndAllSuperInterfaces
.whereType<ClassElement>()
.singleWhereOrNull((c) => c.supertype?.element == propsClass && '_\$${c.name}' == propsClass.name);
// If we can't find the companion, return false, since it won't show up in the forwarding config.
if (companion == null) return false;
propsClass = companion;
}

return !forwardingConfig.excludesProps(propsClass) &&
propsClassBeingForwarded.thisAndAllSuperInterfaces.contains(propsClass);
}

@override
String toString() => 'Forwards props from ${propsClassBeingForwarded.name}: ${forwardingConfig ?? '(unresolved)'}';
}

extension on InterfaceElement {
/// This interface and all its superinterfaces.
///
/// Computed lazily, since [allSupertypes] is expensive.
Iterable<InterfaceElement> get thisAndAllSuperInterfaces sync* {
yield this;
yield* allSupertypes.map((s) => s.element);
}
}

/// Computes and returns forwarded props for a given component [usage], or `null` if the usage does not receive any
/// forwarded props.
ForwardedProps? computeForwardedProps(FluentComponentUsage usage) {
// Lazy variables for potentially expensive values that may get used in multiple loop iterations.
late final enclosingComponentPropsClass =
getTypeOfPropsInEnclosingInterface(usage.node)?.typeOrBound.element.tryCast<InterfaceElement>();

for (final invocation in usage.cascadedMethodInvocations) {
final methodName = invocation.methodName.name;
final arg = invocation.node.argumentList.arguments.firstOrNull;

if (methodName == 'addProps' || methodName == 'modifyProps') {
// If props are conditionally forwarded, don't count them.
final hasConditionArg = invocation.node.argumentList.arguments.length > 1;
if (hasConditionArg) continue;
}

final isAddAllOrAddProps = methodName == 'addProps' || methodName == 'addAll';

// ..addProps(props)
if (isAddAllOrAddProps && arg != null && isPropsFromRender(arg)) {
final propsType = arg.staticType?.typeOrBound.tryCast<InterfaceType>()?.element;
if (propsType != null) {
return ForwardedProps(propsType, PropForwardingConfig.all(), invocation.node);
}
} else if (
// ..addProps(props.getPropsToForward(...))
(isAddAllOrAddProps && arg is MethodInvocation && arg.methodName.name == 'getPropsToForward') ||
// ..modifyProps(props.addPropsToForward(...))
(methodName == 'modifyProps' && arg is MethodInvocation && arg.methodName.name == 'addPropsToForward')) {
final realTarget = arg.realTarget;
if (realTarget != null && isPropsFromRender(realTarget)) {
final propsType = realTarget.staticType?.typeOrBound.tryCast<InterfaceType>()?.element;
if (propsType != null) {
return ForwardedProps(propsType, parsePropsToForwardMethodArgs(arg.argumentList, propsType), invocation.node);
}
}
} else if (
// ..addProps(copyUnconsumedProps())
(isAddAllOrAddProps && arg is MethodInvocation && arg.methodName.name == 'copyUnconsumedProps') ||
// ..modifyProps(addUnconsumedProps)
(methodName == 'modifyProps' && arg is Identifier && arg.name == 'addUnconsumedProps')) {
if (enclosingComponentPropsClass != null) {
return ForwardedProps(
enclosingComponentPropsClass, parseEnclosingClassComponentConsumedProps(usage.node), invocation.node);
}
} else if (
// ..addUnconsumedProps(props, consumedProps)
methodName == 'addUnconsumedProps') {
final consumedPropsArg = invocation.node.argumentList.arguments.elementAtOrNull(1);
if (arg != null && consumedPropsArg != null && isPropsFromRender(arg)) {
final propsType = arg.staticType?.typeOrBound.tryCast<InterfaceType>()?.element;
if (propsType != null) {
return ForwardedProps(propsType, parseConsumedProps(consumedPropsArg), invocation.node);
}
}
}
}

return null;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import 'package:analyzer/dart/element/element.dart';

/// A representation of an over_react consumer's configuration of which props classes to
/// include or exclude when forwarding props.
abstract class PropForwardingConfig {
const PropForwardingConfig();

const factory PropForwardingConfig.all() = _PropForwardingConfig$AllExceptFor;

const factory PropForwardingConfig.allExceptFor(Set<InterfaceElement> onlyProps) = _PropForwardingConfig$AllExceptFor;

const factory PropForwardingConfig.only(Set<InterfaceElement> excludedProps) = _PropForwardingConfig$Only;

/// Whether this configuration might exclude props declared in the props class [e] when forwarding.
bool excludesProps(InterfaceElement e);

String get debugDescription;

@override
toString() => '$debugDescription';
}

class _PropForwardingConfig$Only extends PropForwardingConfig {
final Set<InterfaceElement> _onlyProps;

const _PropForwardingConfig$Only(this._onlyProps);

@override
bool excludesProps(InterfaceElement e) => !_onlyProps.contains(e);

@override
String get debugDescription => 'only props from ${_onlyProps.map((e) => e.name).toSet()}';
}

class _PropForwardingConfig$AllExceptFor extends PropForwardingConfig {
final Set<InterfaceElement> _excludedProps;

const _PropForwardingConfig$AllExceptFor([this._excludedProps = const {}]);

@override
bool excludesProps(InterfaceElement e) => _excludedProps.contains(e);

@override
String get debugDescription =>
_excludedProps.isEmpty ? 'all props' : 'all except props from ${_excludedProps.map((e) => e.name).toSet()}';
}
Loading

0 comments on commit 8f105ce

Please sign in to comment.