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

[greglittlefield-wf]

Motivation

The lint for missing required props lints when required props aren't set.

However, there are some common cases where required props aren't explicitly set, but instead get forwarded from wrapper components, which have their own required prop validation.

So, we should update the lint to not warn when we know certain props are getting forwarded.

For example... (click to expand)

Take the following component and a custom wrapper around it:

mixin UserChipProps on UiProps {
  late User user;
  bool? isSelected;
}

late UiFactory<UserChipProps> UserChip = /*...*/;

mixin CustomUserChipPropsMixin {
  String? customProp;
}

class CustomUserChipProps = UiProps with UserChipProps, CustomUserChipPropsMixin;

UiFactory<CustomUserChipProps> CustomUserChip = uiForwardRef((props, ref) {
  // This UserChip usage has a warning:
  //    Missing required late prop 'user' from 'UserChipProps'. (over_react_late_required_prop)
  return (UserChip()
    ..addProps(props.getPropsToForward(exclude: {CustomUserChipPropsMixin}))
    ..ref = ref
  )(props.children, props.customProp);
}, _$CustomUserChipConfig);

CustomUserChip forwards all its props to the underlying UserChip, including the required prop UserChipProps.user. If we consumed the wrapper component and failed to set that prop, wed'd get a warning like we'd expect:

example() {
  // Warning: Missing required late prop 'user' from 'UserChipProps'. (over_react_late_required_prop)
  CustomUserChip()(); 
}

However, we don't need that first warning when rendering UserChip inside of CustomUserChip's render function, since it passes along those forwarded props, and also validates that they're all set.

Changes

• Add code under lib/src/util/prop_forwarding to determine which props are being forwarded, for all supported prop forwarding syntaxes (we have too many of these 😓)
  • Add extensive tests under unit/util/prop_forwarding_test.dart
• When validating required props, check whether they're being forwarded before linting
  • Add test cases to required props diagnostic
  • Update debug: over_react_required_props debug comment with prop requiredness info (see playground example)
• Add playground example

Release Notes

• Don't lint for required props that are specified due to prop forwarded

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:

QA Checklist

• Tests were updated and provide good coverage of the changeset and other affected code
• Manual testing was performed if needed
  • Steps from PR author:
    • Test coverage looks good and CI passes
    • Test in updated playground file:
      • Prop requiredness behaves as expected in the playground example based on whether props are forwarded
      • Info in // debug: over_react_required_props looks good
  • Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

• A Frontend Frameworks Design member has reviewed these changes
• There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
• For release PRs - Version metadata in Rosie comment is correct

[aviary3-wk]

Security Insights

No security relevant content was detected by automated scans.

Action Items

• Review PR for security impact; comment "security review required" if needed or unsure
• Verify aviary.yaml coverage of security relevant code

---

Questions or Comments? Reach out on Slack: #support-infosec.

[greglittlefield-wf]

Couldn't get the formatter to not add these empty spaces 🤷

[greglittlefield-wf]

Went with replaceAll

• so I could use raw strings and not have to escape the $s in the boilerplate lol
• so that the language=dart language injection wouldn't get messed up by string interpolations

[greglittlefield-wf]

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

[aaronlademann-wf]

Didn't quite make it all the way through. Will finish this up in the morning

[aaronlademann-wf]

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

[greglittlefield-wf]

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

[aaronlademann-wf]

^ 🎯

[aaronlademann-wf]

+1

🤩 test coverage

[aaronlademann-wf]

QA +1

• CI Passing
• Tested the plugin playground locally, linting worked as expected - as did the debugging

@Workiva/release-management-pp

[rmconsole-wf]

+1 from RM