-
Notifications
You must be signed in to change notification settings - Fork 58
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-3159 Fix required prop null-safe-conditional linting #948
FED-3159 Fix required prop null-safe-conditional linting #948
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
@@ -138,16 +140,26 @@ class MissingRequiredPropTest_NoErrors extends MissingRequiredPropTest { | |||
test() => InheritsLateRequired()(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This existing test case (test_noErrorsInheritsLateRequired
) and the newly-added regression test should, together, validate the behavior we want to see.
@@ -220,7 +219,8 @@ class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor | |||
continue; | |||
} | |||
|
|||
if (withNullability(result) || requiredness != PropRequiredness.late) { | |||
// Late required prop validation is only enabled for props classes that have migrated to null safety. | |||
if (propsClassElement.library.isNonNullableByDefault || requiredness != PropRequiredness.late) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main fix; instead of checking result.library
, we're now checking propsClassElement.library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA +1
- regression test used to fail and now it passes with the fix
- I was also using the branch yesterday in some consumer repos and saw expected warnings show up on this branch that didn't show up when using over_react master
@Workiva/release-management-p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from RM
Motivation
The over_react analyzer plugin is always supposed to lint for missing required props, unless they're on props declared in non-null safe libraries.
There's a bug in this conditional that instead checks the language version of the library the component is consumed in, as opposed to the library the component is declared in.
This prevents consumers from getting static warnings in cases where they'll get runtime errors.
For example, expected behavior:
vs actual behavior:
Changes
withNullability
util with theLibraryElement.isNonNullableByDefault
API, which I just learned about!Release Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: