FED-2156 Unsafe required props lint

[greglittlefield-wf]

Motivation

While required late props are validated to be present when a component is rendered, there are cases where they won't always be present. For example, "partial" props objects that represent a subset of a component's props.

If required late props are accessed without being set, they'll throw at runtime, and there's currently no static analysis that prevents consumers from doing so, or notifies them that they're doing something unsafe.

So, we want to help prevent consumers from unsafely accessing required late props.

Changes

• Add new UnsafeRequiredPropAccessDiagnostic that warns whenever required props are accessed in an unsafe manner
  • Lints when accessing required props on objects that aren't props a component was rendered with
  • Exceptions where we don't lint
    • Accesses on callback args in utility methods like .getPropKey((p) => p.requiredProp)
    • Within memo areEqual functions, or top-level functions with areEqual in the name since consumers apparently split themout a fair bit
    • In files within the test/ directory, which typically uses getProps a lot (and if unsafe accesses happen, they'll just fail tests instead of crashing an app)
    • Within lifecycle methods that receive props as an argument and typically use typedPropsFactory
      • We could track these down and ignore them only if they're safe, but that didn't seem worth the effort/complexity
• Add tests
  • Update SharedAnalysisContext to allow creation of files outside of lib
• Add playground example

Release Notes

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:
    • Verify CI passes
    • Manually test
      • Test in playground example
      • Test in a consumer that uses required props (e.g., FluxUiProps) and verify there aren't any unexpected errors
      • Double-check with // debug: over_react_metrics that performance is acceptable (this lint has to process all property accesses to see if they're on props). From my testing it looked pretty efficient, all things considered, and was hovering around being our 5th slowest lint, which IMO isn't bad at all.
  • 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.

[sydneyjodon-wk]

Is there a reason why the other functions like this are private, but this one isn't?

[greglittlefield-wf]

Oh nope, must have just missed this one while cleaning up. Made private in 71431c7.

[sydneyjodon-wk]

Was there going to be a comment here?

[greglittlefield-wf]

Oops, missed this one. Added in 71431c7.

[sydneyjodon-wk]

This is super awesome, Greg! You were so thorough!!!!

The only thing that came up in QA was that on my computer, // debug:over_react_metrics in the playground has consistently shown UnsafeRequiredPropAccessDiagnostic to be one of the two slowest - at first it was always the slowest at around 44%, but now whenever I restart my analysis server, it's consistently section slowest:

  ** UnsafeRequiredPropAccessDiagnostic: 3.483ms (24.9%)
  ** UnsafeRequiredPropAccessDiagnostic: 3.270ms (25.2%),
  ** UnsafeRequiredPropAccessDiagnostic: 3.488ms (25.1%),
  ** UnsafeRequiredPropAccessDiagnostic: 3.344ms (25.3%),

I don't know if that's much different from what you were seeing since you said it was around fifth slowest?

Edit: although I just tested it a few times in xbrl-module and it wasn't even in the top three so maybe it has more to do with how hard the other diagnostics are working in the playground relative to this one

QA steps:

• Test in playground example
• Test in a consumer that uses required props (e.g., FluxUiProps) and verify there aren't any unexpected errors - tested in xbrl-module
• Double-check with // debug: over_react_metrics that performance is acceptable (this lint has to process all property accesses to see if they're on props).

[greglittlefield-wf]

Edit: although I just tested it a few times in xbrl-module and it wasn't even in the top three so maybe it has more to do with how hard the other diagnostics are working in the playground relative to this one

@sydneyjodon-wk Ah yup, I think I had been testing on larger files in other repos. Yeah that makes sense to me, since most of the playground examples don't have a lot of code for the diagnostics to process.

So yeah as long as performance looks acceptable on larger, real-world code, I think we're good.

[sydneyjodon-wk]

+10 🎉