Skip to content

Commit 8f105ce

Browse files
Merge pull request #944 from Workiva/FED-2034-required-props-lint-respect-forwarded
FED-2034: Analyzer plugin required props validation: check forwarded props
2 parents 6884cd8 + 5e37b83 commit 8f105ce

File tree

11 files changed

+1576
-5
lines changed

11 files changed

+1576
-5
lines changed

tools/analyzer_plugin/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ Currently-available debug info:
120120
builder and analyzer functionality dealing with component declarations
121121
- `// debug: over_react_metrics` - shows performance data on how long diagnostics took to run
122122
- `// debug: over_react_exhaustive_deps` - shows info on how dependencies were detected/interpreted
123-
- `// debug: over_react_required_props` - shows requiredness info for all props for each builder
123+
- `// debug: over_react_required_props` - shows requiredness info for all props for each builder, as well as prop forwarding info
124124

125125
#### Attaching a Debugger
126126
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.

tools/analyzer_plugin/analysis_options.yaml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ include: package:workiva_analysis_options/v1.recommended.yaml
33
analyzer:
44
exclude:
55
- playground/**
6+
- test/temporary_test_fixtures/**
67
- test/test_fixtures/**
78
strong-mode:
89
implicit-casts: false

tools/analyzer_plugin/lib/src/diagnostic/missing_required_prop.dart

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import 'package:over_react_analyzer_plugin/src/util/ast_util.dart';
77
import 'package:over_react_analyzer_plugin/src/util/null_safety_utils.dart';
88
import 'package:over_react_analyzer_plugin/src/util/pretty_print.dart';
99
import 'package:over_react_analyzer_plugin/src/util/prop_declarations/props_set_by_factory.dart';
10+
import 'package:over_react_analyzer_plugin/src/util/prop_forwarding/forwarded_props.dart';
1011
import 'package:over_react_analyzer_plugin/src/util/util.dart';
1112
import 'package:over_react_analyzer_plugin/src/util/weak_map.dart';
1213

@@ -67,6 +68,7 @@ class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor
6768
"Missing required late prop {0}.",
6869
AnalysisErrorSeverity.WARNING,
6970
AnalysisErrorType.STATIC_WARNING,
71+
correction: _correctionMessage,
7072
);
7173

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

84+
static const _correctionMessage =
85+
"Either set this prop, or mix it into the enclosing component's props and forward it.";
86+
8187
static DiagnosticCode _codeForRequiredness(PropRequiredness requiredness) {
8288
switch (requiredness) {
8389
case PropRequiredness.late:
@@ -168,8 +174,9 @@ class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor
168174
final debugHelper = AnalyzerDebugHelper(result, collector, enabled: _cachedIsDebugHelperEnabled(result));
169175
// A flag to help verify during debugging/testing whether propsSetByFactory was computed.
170176
var hasPropsSetByFactoryBeenComputed = false;
177+
final debugSuppressedRequiredPropsDueToForwarding = <FieldElement>{};
171178

172-
// Use a late variable to compute this only when we need to.
179+
// Use late variables to compute these only when we need to.
173180
late final propsSetByFactory = () {
174181
hasPropsSetByFactoryBeenComputed = true;
175182

@@ -181,8 +188,8 @@ class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor
181188

182189
return _cachedGetPropsSetByFactory(factoryElement);
183190
}();
184-
185-
final presentPropNames =
191+
late final forwardedProps = computeForwardedProps(usage);
192+
late final presentPropNames =
186193
usage.cascadedProps.where((prop) => !prop.isPrefixed).map((prop) => prop.name.name).toSet();
187194

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

201-
// TODO(FED-2034) don't warn when we know required props are being forwarded
208+
final sourcePropsClass = field.enclosingElement;
209+
if (sourcePropsClass is InterfaceElement) {
210+
if (forwardedProps != null && forwardedProps.definitelyForwardsPropsFrom(sourcePropsClass)) {
211+
if (debugHelper.enabled) {
212+
debugSuppressedRequiredPropsDueToForwarding.add(field);
213+
}
214+
continue;
215+
}
216+
}
202217

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

239+
if (forwardedProps != null) {
240+
debugHelper.log(() {
241+
var message = StringBuffer()..writeln(forwardedProps);
242+
if (debugSuppressedRequiredPropsDueToForwarding.isNotEmpty) {
243+
final propsNamesByClassName = <String?, Set<String>>{};
244+
for (final field in debugSuppressedRequiredPropsDueToForwarding) {
245+
propsNamesByClassName.putIfAbsent(field.enclosingElement.name, () => {}).add(field.name);
246+
}
247+
message.write('Required props set only via forwarding: ${prettyPrint(propsNamesByClassName)}');
248+
} else {}
249+
return message.toString();
250+
}, () => result.locationFor(forwardedProps.debugSourceNode));
251+
}
252+
224253
// Include debug info for each invocation ahout all the props and their requirednesses.
225254
debugHelper.log(() {
226255
final propNamesByRequirednessName = <String, Set<String>>{};
Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
import 'package:analyzer/dart/ast/ast.dart';
2+
import 'package:analyzer/dart/element/element.dart';
3+
import 'package:analyzer/dart/element/type.dart';
4+
import 'package:over_react_analyzer_plugin/src/diagnostic_contributor.dart';
5+
import 'package:over_react_analyzer_plugin/src/util/ast_util.dart';
6+
import 'package:over_react_analyzer_plugin/src/util/is_props_from_render.dart';
7+
import 'package:over_react_analyzer_plugin/src/util/prop_forwarding/parse_forwarding_config.dart';
8+
import 'package:over_react_analyzer_plugin/src/util/util.dart';
9+
10+
import 'forwarding_config.dart';
11+
import 'util.dart';
12+
13+
/// A representation of props forwarded to a component usage.
14+
class ForwardedProps {
15+
/// The props class that props are being forwarded from.
16+
///
17+
/// For example, the type of `props` in `..addUnconsumedProps(props, ...)`.
18+
final InterfaceElement propsClassBeingForwarded;
19+
20+
/// The configuration of which props to forward, or null if it could not be resolved.
21+
final PropForwardingConfig? forwardingConfig;
22+
23+
/// A node that represents the addition of forwarded props, for use in debug infos only.
24+
final AstNode debugSourceNode;
25+
26+
ForwardedProps(this.propsClassBeingForwarded, this.forwardingConfig, this.debugSourceNode);
27+
28+
/// Returns whether these forwarded props definitely include props from [propsClass], or false
29+
/// if forwarded props could not be resolved.
30+
///
31+
/// This is true only when all of the following conditions are met:
32+
/// - [propsClassBeingForwarded] inherits from [propsClass] (i.e., is or mixes in those props)
33+
/// - [propsClass] is not excluded by [forwardingConfig]
34+
bool definitelyForwardsPropsFrom(InterfaceElement propsClass) {
35+
final forwardingConfig = this.forwardingConfig;
36+
if (forwardingConfig == null) return false;
37+
38+
// Handle legacy classes being passed in.
39+
if (propsClass.name.startsWith(r'_$')) {
40+
// Look up the companion and use that instead, since that's what will be referenced in the forwarding config.
41+
// E.g., for `_$FooProps`, find `FooProps`, since consumers will be using `FooProps` when setting up prop forwarding.
42+
final companion = propsClassBeingForwarded.thisAndAllSuperInterfaces
43+
.whereType<ClassElement>()
44+
.singleWhereOrNull((c) => c.supertype?.element == propsClass && '_\$${c.name}' == propsClass.name);
45+
// If we can't find the companion, return false, since it won't show up in the forwarding config.
46+
if (companion == null) return false;
47+
propsClass = companion;
48+
}
49+
50+
return !forwardingConfig.excludesProps(propsClass) &&
51+
propsClassBeingForwarded.thisAndAllSuperInterfaces.contains(propsClass);
52+
}
53+
54+
@override
55+
String toString() => 'Forwards props from ${propsClassBeingForwarded.name}: ${forwardingConfig ?? '(unresolved)'}';
56+
}
57+
58+
extension on InterfaceElement {
59+
/// This interface and all its superinterfaces.
60+
///
61+
/// Computed lazily, since [allSupertypes] is expensive.
62+
Iterable<InterfaceElement> get thisAndAllSuperInterfaces sync* {
63+
yield this;
64+
yield* allSupertypes.map((s) => s.element);
65+
}
66+
}
67+
68+
/// Computes and returns forwarded props for a given component [usage], or `null` if the usage does not receive any
69+
/// forwarded props.
70+
ForwardedProps? computeForwardedProps(FluentComponentUsage usage) {
71+
// Lazy variables for potentially expensive values that may get used in multiple loop iterations.
72+
late final enclosingComponentPropsClass =
73+
getTypeOfPropsInEnclosingInterface(usage.node)?.typeOrBound.element.tryCast<InterfaceElement>();
74+
75+
for (final invocation in usage.cascadedMethodInvocations) {
76+
final methodName = invocation.methodName.name;
77+
final arg = invocation.node.argumentList.arguments.firstOrNull;
78+
79+
if (methodName == 'addProps' || methodName == 'modifyProps') {
80+
// If props are conditionally forwarded, don't count them.
81+
final hasConditionArg = invocation.node.argumentList.arguments.length > 1;
82+
if (hasConditionArg) continue;
83+
}
84+
85+
final isAddAllOrAddProps = methodName == 'addProps' || methodName == 'addAll';
86+
87+
// ..addProps(props)
88+
if (isAddAllOrAddProps && arg != null && isPropsFromRender(arg)) {
89+
final propsType = arg.staticType?.typeOrBound.tryCast<InterfaceType>()?.element;
90+
if (propsType != null) {
91+
return ForwardedProps(propsType, PropForwardingConfig.all(), invocation.node);
92+
}
93+
} else if (
94+
// ..addProps(props.getPropsToForward(...))
95+
(isAddAllOrAddProps && arg is MethodInvocation && arg.methodName.name == 'getPropsToForward') ||
96+
// ..modifyProps(props.addPropsToForward(...))
97+
(methodName == 'modifyProps' && arg is MethodInvocation && arg.methodName.name == 'addPropsToForward')) {
98+
final realTarget = arg.realTarget;
99+
if (realTarget != null && isPropsFromRender(realTarget)) {
100+
final propsType = realTarget.staticType?.typeOrBound.tryCast<InterfaceType>()?.element;
101+
if (propsType != null) {
102+
return ForwardedProps(propsType, parsePropsToForwardMethodArgs(arg.argumentList, propsType), invocation.node);
103+
}
104+
}
105+
} else if (
106+
// ..addProps(copyUnconsumedProps())
107+
(isAddAllOrAddProps && arg is MethodInvocation && arg.methodName.name == 'copyUnconsumedProps') ||
108+
// ..modifyProps(addUnconsumedProps)
109+
(methodName == 'modifyProps' && arg is Identifier && arg.name == 'addUnconsumedProps')) {
110+
if (enclosingComponentPropsClass != null) {
111+
return ForwardedProps(
112+
enclosingComponentPropsClass, parseEnclosingClassComponentConsumedProps(usage.node), invocation.node);
113+
}
114+
} else if (
115+
// ..addUnconsumedProps(props, consumedProps)
116+
methodName == 'addUnconsumedProps') {
117+
final consumedPropsArg = invocation.node.argumentList.arguments.elementAtOrNull(1);
118+
if (arg != null && consumedPropsArg != null && isPropsFromRender(arg)) {
119+
final propsType = arg.staticType?.typeOrBound.tryCast<InterfaceType>()?.element;
120+
if (propsType != null) {
121+
return ForwardedProps(propsType, parseConsumedProps(consumedPropsArg), invocation.node);
122+
}
123+
}
124+
}
125+
}
126+
127+
return null;
128+
}
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
import 'package:analyzer/dart/element/element.dart';
2+
3+
/// A representation of an over_react consumer's configuration of which props classes to
4+
/// include or exclude when forwarding props.
5+
abstract class PropForwardingConfig {
6+
const PropForwardingConfig();
7+
8+
const factory PropForwardingConfig.all() = _PropForwardingConfig$AllExceptFor;
9+
10+
const factory PropForwardingConfig.allExceptFor(Set<InterfaceElement> onlyProps) = _PropForwardingConfig$AllExceptFor;
11+
12+
const factory PropForwardingConfig.only(Set<InterfaceElement> excludedProps) = _PropForwardingConfig$Only;
13+
14+
/// Whether this configuration might exclude props declared in the props class [e] when forwarding.
15+
bool excludesProps(InterfaceElement e);
16+
17+
String get debugDescription;
18+
19+
@override
20+
toString() => '$debugDescription';
21+
}
22+
23+
class _PropForwardingConfig$Only extends PropForwardingConfig {
24+
final Set<InterfaceElement> _onlyProps;
25+
26+
const _PropForwardingConfig$Only(this._onlyProps);
27+
28+
@override
29+
bool excludesProps(InterfaceElement e) => !_onlyProps.contains(e);
30+
31+
@override
32+
String get debugDescription => 'only props from ${_onlyProps.map((e) => e.name).toSet()}';
33+
}
34+
35+
class _PropForwardingConfig$AllExceptFor extends PropForwardingConfig {
36+
final Set<InterfaceElement> _excludedProps;
37+
38+
const _PropForwardingConfig$AllExceptFor([this._excludedProps = const {}]);
39+
40+
@override
41+
bool excludesProps(InterfaceElement e) => _excludedProps.contains(e);
42+
43+
@override
44+
String get debugDescription =>
45+
_excludedProps.isEmpty ? 'all props' : 'all except props from ${_excludedProps.map((e) => e.name).toSet()}';
46+
}

0 commit comments

Comments
 (0)