FED-2112 Required props lint: respect @Props(ignoreRequiredProps: ...) #873

greglittlefield-wf · 2024-01-22T23:56:36Z

Dependent PR

These changes are dependent on two different PRs:

For review, I've set the base as a branch that merges the two together.

Motivation

Part of adding ignoreRequiredProps functionality is supporting it in the analyzer plugin, by making the required props lint not treat those props as required.

Changes

Update getAllRequiredProps logic to mark props specified within @Props(ignoreRequiredProps: ...) as ignored
- Add unit tests
- Add diagnostic integration tests
- Add playground example
Add ignore for invalid_use_of_visible_for_overriding_member to generated code, since it was causing analyzer plugin test setups to fail when verifying that there are no analysis errors in generated parts
- Update generated code and golds
Update boilerplate parsing to not throw when failing to instantiate an annotation, and instead severe-log.
- That way, analyzer plugin functionality that relies on boilerplate parsing doesn't emit an IDE error notification when typing an annotation (I noticed this while QAing this change):
- Add regression test

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 in playground (tools/analyzer_plugin/playground/web/missing_required_props.dart) that:
    - the added IgnoresSomeRequired example doesn't warn for props that are ignored
    - adding/removing the ignoreRequiredProps argument doesn't cause IDE error notifications as you're typing
- 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

…2' into analyzer-plugin-opt-out-required-props

aviary3-wk · 2024-01-22T23:57:45Z

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 · 2024-01-23T00:00:30Z

lib/src/builder/builder.dart

+    'unnecessary_null_in_if_null_operators',
+    'prefer_null_aware_operators',
+    // Needed to ignore the `requiredPropNamesToSkipValidation` checks.
+    'invalid_use_of_visible_for_overriding_member',


I added this ignore to fix my test setup, which validated there were no warnings in generated code

…-opt-out-required-props--clean-base

…2' into analyzer-plugin-opt-out-required-props--clean-base

… for now

Needed for usages of requiredPropNamesToSkipValidation

…ilerplate parsing

greglittlefield-wf · 2024-01-23T20:57:49Z

lib/src/builder/parsing/members/props_and_state_util.dart

@@ -50,32 +50,54 @@ class _PropsStateStringHelpersImpl extends Object with PropsStateStringHelpers {
  _PropsStateStringHelpersImpl({required this.isProps});
 }

-final Expando<String> props_ignoreRequiredProps_source = Expando();
-
 /// Uses [InstantiatedMeta] to analyze [node] and determine the proper annotation.
 annotations.TypedMap getPropsOrStateAnnotation(bool isProps, AnnotatedNode node) {


Whitespace-agnostic diff here is helpful

greglittlefield-wf · 2024-01-23T20:57:57Z

lib/src/builder/parsing/meta.dart

@@ -102,31 +105,38 @@ class InstantiatedComponentMeta<TMeta> extends InstantiatedMeta<TMeta> {
  final Identifier? subtypeOfValue;

  InstantiatedComponentMeta._(
-      Annotation? metaNode, TMeta meta, List<Expression> unsupportedArguments, this.subtypeOfValue)
+      Annotation metaNode, TMeta meta, List<Expression> unsupportedArguments, this.subtypeOfValue)
      : super._(metaNode, meta, unsupportedArguments);

  static InstantiatedComponentMeta<T>? fromNode<T>(AnnotatedNode node) {


Whitespace-agnostic diff here is helpful

sydneyjodon-wk

+10

smoke tested in playground

…-out-required-props

sydneyjodon-wk

+10 refresh

Merge remote-tracking branch 'origin/FED-2015-opt-out-required-props-…

939993f

…2' into analyzer-plugin-opt-out-required-props

greglittlefield-wf marked this pull request as ready for review January 22, 2024 23:57

rmconsole-wf added the Merge Requirements Unmet label Jan 22, 2024

greglittlefield-wf commented Jan 23, 2024

View reviewed changes

greglittlefield-wf requested review from aaronlademann-wf and sydneyjodon-wk January 23, 2024 00:04

rmconsole-wf added the Skynet 2.0 Failed label Jan 23, 2024

Merge branch 'FED-2015-opt-out-required-props-2' into analyzer-plugin…

e0c0f45

…-opt-out-required-props--clean-base

greglittlefield-wf force-pushed the analyzer-plugin-opt-out-required-props branch from 7510775 to cbec88a Compare January 23, 2024 16:33

greglittlefield-wf added 9 commits January 23, 2024 13:44

Merge remote-tracking branch 'origin/FED-2015-opt-out-required-props-…

4846adf

…2' into analyzer-plugin-opt-out-required-props--clean-base

Respect ignoreRequiredProps in analyzer plugin, add tests

b2a1191

Ignore invalid_use_of_visible_for_overriding_member in generated code…

c859acb

… for now

Add invalid_use_of_visible_for_overriding_member in generated code

77e3ec5

Needed for usages of requiredPropNamesToSkipValidation

Remove invalid_use_of_visible_for_overriding_member workaround

008939c

Add some missing test cases

92a2e82

Add integration test for ignored props

ef7d8d2

Update playground

b97d9ad

Add regression test when typing boilerplate annotations

8a2e187

greglittlefield-wf force-pushed the analyzer-plugin-opt-out-required-props branch from cbec88a to 8a2e187 Compare January 23, 2024 20:44

Severe-log instead of throwing when failing to instantiate meta in bo…

56d5c97

…ilerplate parsing

greglittlefield-wf commented Jan 23, 2024

View reviewed changes

greglittlefield-wf added 4 commits January 23, 2024 14:03

Check in generated code changes (added ignore_for_file)

a8e401a

Update gold files with added ignore_for_file

fb68796

Update tests after making metaNode non-nullable

6b1e1c7

Update subtypeOf error test

14d6c32

greglittlefield-wf force-pushed the analyzer-plugin-opt-out-required-props branch from 1d33d19 to 14d6c32 Compare January 23, 2024 21:31

rmconsole-wf removed the Skynet 2.0 Failed label Jan 23, 2024

sydneyjodon-wk approved these changes Jan 24, 2024

View reviewed changes

greglittlefield-wf changed the base branch from analyzer-plugin-opt-out-required-props--clean-base to v5_wip January 25, 2024 20:54

Merge remote-tracking branch 'origin/v5_wip' into analyzer-plugin-opt…

3dc54cf

…-out-required-props

sydneyjodon-wk approved these changes Jan 25, 2024

View reviewed changes

greglittlefield-wf merged commit a677009 into v5_wip Jan 25, 2024
8 checks passed

greglittlefield-wf deleted the analyzer-plugin-opt-out-required-props branch January 25, 2024 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FED-2112 Required props lint: respect @Props(ignoreRequiredProps: ...) #873

FED-2112 Required props lint: respect @Props(ignoreRequiredProps: ...) #873

greglittlefield-wf commented Jan 22, 2024 •

edited

Loading

aviary3-wk commented Jan 22, 2024

greglittlefield-wf Jan 23, 2024

greglittlefield-wf Jan 23, 2024

greglittlefield-wf Jan 23, 2024

sydneyjodon-wk left a comment

sydneyjodon-wk left a comment

FED-2112 Required props lint: respect @Props(ignoreRequiredProps: ...) #873

FED-2112 Required props lint: respect @Props(ignoreRequiredProps: ...) #873

Conversation

greglittlefield-wf commented Jan 22, 2024 • edited Loading

Dependent PR

Motivation

Changes

Release Notes

Review

QA Checklist

Merge Checklist

aviary3-wk commented Jan 22, 2024

Security Insights

Action Items

greglittlefield-wf Jan 23, 2024

Choose a reason for hiding this comment

greglittlefield-wf Jan 23, 2024

Choose a reason for hiding this comment

greglittlefield-wf Jan 23, 2024

Choose a reason for hiding this comment

sydneyjodon-wk left a comment

Choose a reason for hiding this comment

sydneyjodon-wk left a comment

Choose a reason for hiding this comment

greglittlefield-wf commented Jan 22, 2024 •

edited

Loading