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

FED-2034: Analyzer plugin required props validation: check forwarded props #944

Merged
merged 43 commits into from
Sep 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
2232681
Detect forwarded props, don't lint for forwarded required props
greglittlefield-wf Aug 21, 2024
e7fbe54
Remove duplicate, outdated isPropsFromRender implementation
greglittlefield-wf Aug 21, 2024
645c551
Add comments for cases
greglittlefield-wf Aug 21, 2024
dc02ee9
Implement case: `..addUnconsumedProps(props, ...)`
greglittlefield-wf Aug 21, 2024
7b6fb97
Include props.staticMeta case when looking up forwarded props
greglittlefield-wf Aug 26, 2024
a38d70b
Add some tests for getForwardedProps
greglittlefield-wf Aug 27, 2024
7404c80
Remove unused typeSystem args
greglittlefield-wf Aug 27, 2024
a427fcc
Add cases for forwarding all props, fix addAll case
greglittlefield-wf Aug 27, 2024
bf0130a
Clean up test cases
greglittlefield-wf Aug 28, 2024
4c3cbca
Use clearer names in test cases
greglittlefield-wf Aug 28, 2024
b410651
Test multiple props in sets
greglittlefield-wf Aug 28, 2024
f04896b
Add, organize test cases
greglittlefield-wf Aug 28, 2024
1e3f41e
Refactor PropsToForward implementation to be less dynamic
greglittlefield-wf Aug 29, 2024
879e07d
Reorganize prop forwarding code into multiple files, improve naming
greglittlefield-wf Aug 29, 2024
cfb1b4c
Naming/comment cleanup
greglittlefield-wf Aug 29, 2024
cd7bc63
Add more test cases, mostly for class components
greglittlefield-wf Aug 29, 2024
d9a16d2
Handle legacy class components without consumedProps, add test
greglittlefield-wf Aug 29, 2024
3aa6538
Add negative test cases
greglittlefield-wf Sep 6, 2024
28c5d4d
DRY up addAll/addProps tests
greglittlefield-wf Sep 6, 2024
a918af7
Add static meta variable test cases, fix inverted boolean
greglittlefield-wf Sep 9, 2024
42c577d
Clean up variables and debug code
greglittlefield-wf Sep 9, 2024
7705bec
Add doc comment
greglittlefield-wf Sep 9, 2024
6fa947e
Add some test cases for new prop forwarding logic in diagnostic
greglittlefield-wf Sep 9, 2024
bf87c7a
Add tests for legacy props edge case
greglittlefield-wf Sep 10, 2024
61ee0ba
Add workaround for legacy props
greglittlefield-wf Sep 10, 2024
fecb9b1
Add companion normalization case, regression test for same type case
greglittlefield-wf Sep 10, 2024
295a58e
Fix false negative when propClassBeingForwarded == propsClass
greglittlefield-wf Sep 10, 2024
623fc3f
Add missing analyzer exclude
greglittlefield-wf Sep 10, 2024
3250872
Remove TODO, move forwarding check to after cheaper short-circuit
greglittlefield-wf Sep 10, 2024
98adca6
Add doc comments improve name
greglittlefield-wf Sep 10, 2024
0c7ec4c
Reference prop forwarding in lint tooltip
greglittlefield-wf Sep 11, 2024
53d7f3d
Replace unresolved case with null
greglittlefield-wf Sep 11, 2024
21c0b5c
Clean up naming, add doc comments
greglittlefield-wf Sep 11, 2024
6f890e1
Expect resolved forwarding configs in test cases
greglittlefield-wf Sep 11, 2024
954841e
Update debug comment description
greglittlefield-wf Sep 11, 2024
113418e
Remove unnecessary null check leftover from unresolved class removal
greglittlefield-wf Sep 11, 2024
2410b56
Add unresolved forwarding config test cases
greglittlefield-wf Sep 11, 2024
835ca92
Use more verbose test variable name since it no longer affects wrapping
greglittlefield-wf Sep 11, 2024
9125142
Add required props forwarding example to playground
greglittlefield-wf Sep 12, 2024
d0aca29
Consolidate "all" forwarding config case
greglittlefield-wf Sep 12, 2024
943032f
Clean up and improve prop forwarding debug infos, add demo
greglittlefield-wf Sep 12, 2024
0aa15fd
Add test cases for conditional forwarding
greglittlefield-wf Sep 12, 2024
5e37b83
Bail out when props are conditionally forwarded
greglittlefield-wf Sep 12, 2024
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
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>>{};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a null key here indicative of things like ubiquitive / un-namespaced props - or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's just that technically .name is nullable here, since field.enclosingElement is typed as Element and not InterfaceElement (which has a non-nullable .name).

So in practice, this should never be null

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 {
Copy link
Contributor Author

@greglittlefield-wf greglittlefield-wf Sep 12, 2024

Choose a reason for hiding this comment

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

I kept going back and forth between combining this class into ForwardedProps, but it seemed cleaner to keep it separate:

  1. from an organizational/conceptual perspective and
  2. because information we need to populate ForwardedProps.propsClassBeingForwarded isn't present everywhere we'd parse one of these configs, and would have to be passed through potentially multiple layers of functions

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