FED-1863 Required props lint #871

greglittlefield-wf · 2024-01-11T00:37:27Z

Dependent PR

This PR is dependent on #870

Motivation

To help provide a good developer experience when working with required late props, we want to provide a lint that warns when a component is missing required props.

That way, devs can get static warnings, and not wait until they hit our runtime errors to find out they were missing props.

This lint should be similar to the warnings you get when you are missing required function parameters.

We had a partial implementation of this lint, and we needed to get it production-ready.

Changes

Finish MissingRequiredPropDiagnostic implementation, enable it, add tests
- This supports both late required and @requiredProp required props, but after some back and forth I opted to only lint for late required by default. For a more detailed explanation, see comment in tools/analyzer_plugin/lib/src/plugin.dart
Add utilities used by the diagnostic, with unit tests
- getAllProps to get all props for a props class and its supertypes
- getAllRequiredProps to collect info on which props are required
- isRequiredPropValidationDisabled
Raise analyzer plugin SDK language constraint to 2.17 to use enhanced enums
Other changes not directly related to this diagnostic
- Work around InconsistentAnalysisExceptions when typing
  - I noticed when manually testing that we'd get a bunch errors in the IDE about InconsistentAnalysisException, unrelated to this PR. I found similar issues documented in Dart SDK tooling, and applied a similar workaround.
- Improve debug: over_react_metrics output to aid in performance optimizing
  - Star the top 3 slowest diagnostics
  - Show getAllComponentUsages overhead
- AnalyzerDebugHelper - make log message and location computation lazy by default, so that we're not slowing down performance when debug flags aren't enabled
  - Update existing usages, add locations in missing_cascade_parens.dart

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:
  - Manually verify that diagnostic behaves as expected in tools/analyzer_plugin/playground/web/missing_required_props.dart
  - Remove the over_react_annotation_required_prop line in the playground's analysis_options.yaml and verify that annotation-required props don't lint by default
  - Try typing in the playground and verify there are virtually no InconsistentAnalysisExceptions errors compared to the base branch
  - Verify behavior of debug flag comments looks good:
    - // debug: over_react_metrics
      - Verify new behavior mentioned in Changes section looks good
      - Verify the performance of this diagnostic looks good
    - // debug: over_react_required_props - shows information about required props for each builder
- 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

…nores

…o required-props-lint

greglittlefield-wf · 2024-01-11T00:59:07Z

tools/analyzer_plugin/lib/src/util/prop_declarations/get_all_props.dart

+import 'package:analyzer/dart/element/element.dart';
+import 'package:over_react_analyzer_plugin/src/util/util.dart';
+
+// Performance optimization notes:


I spent quite a bit of time optimizing this function, and I feel pretty happy about its performance!

In practice, usage is optimized further via the caching done with _cachedGetAllRequiredProps.

greglittlefield-wf · 2024-01-11T18:23:58Z

tools/analyzer_plugin/lib/src/async_plugin_apis/diagnostic.dart

@@ -147,59 +150,71 @@ class _DiagnosticGenerator {
  /// by the given [unitResult]. If any of the contributors throws an exception,
  /// also create a non-fatal 'plugin.error' notification.
  Future<_GeneratorResult<List<AnalysisError>>> generateErrors(ResolvedUnitResult unitResult) async {
-    return _generateErrors(unitResult, DiagnosticCollectorImpl(shouldComputeFixes: false));
+    return _generateErrors(unitResult, DiagnosticCollectorImpl(shouldComputeFixes: false),
+        catchInconsistentAnalysisException: true);
  }

  /// Creates an 'edit.getFixes' response for the location in the file specified
  /// by the given [request]. If any of the contributors throws an exception,
  /// also create a non-fatal 'plugin.error' notification.
  Future<_GeneratorResult<EditGetFixesResult>> generateFixesResponse(DartFixesRequest request) async {


I'd recommend using whitespace-agnostic diff for these diffs

greglittlefield-wf · 2024-01-11T18:24:55Z

tools/analyzer_plugin/lib/src/diagnostic/missing_required_prop.dart

@@ -53,62 +54,121 @@ render() {
 // </editor-fold>

 class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor {


Since this diagnostic always existed but was only partially implemented, was always disabled, and was never fully reviewed, this full file should be reviewed as if it were newly-added, as opposed to reviewing just the diff

Fixes regression tests added in previous commit

greglittlefield-wf · 2024-01-19T22:19:08Z

tools/analyzer_plugin/lib/src/util/prop_declarations/required_props.dart

+  /// Represents a prop that was considered required in its declaration,
+  /// but is ignored by the consuming class.
+  ignoredByConsumingClass(isRequired: false, requirednessLevel: 1),


This is currently unused, but will get used as part of updates to this file in FED-2015.

While working on that, I wanted to come back to this PR to do a little bit of refactoring, and also added requirednessLevel and new PropRequiredness enum test cases, so I figured I'd just stub it in now.

aaronlademann-wf

Couple questions.

Insane test coverage. 🥇

aaronlademann-wf · 2024-01-24T17:31:12Z

tools/analyzer_plugin/lib/src/diagnostic/missing_required_prop.dart

+  /// Returns whether the debug helper should be enabled, caching results per result
+  /// so we don't have to run it in in every [computeErrorsForUsage].
+  ///
+  /// TODO Add shared debug flag computation in DiagnosticContributor/DiagnosticCollector


Does this need to be addressed in this PR?

No, but I would love to circle back to it later on, maybe next time we add a new debug flag

aaronlademann-wf · 2024-01-24T17:32:45Z

tools/analyzer_plugin/lib/src/diagnostic/missing_required_prop.dart

+    // DOM and SVG components are common and have a large number of props, none of which are required;
+    // short-circuit here for performance.
+    if (const {'DomProps', 'SvgProps'}.contains(propsClassElement.name)) return; // [1]


aaronlademann-wf · 2024-01-24T17:37:54Z

tools/analyzer_plugin/playground/analysis_options.yaml

+over_react:
+  errors:
+    # Opt into this diagnostic that's disabled by default.
+    over_react_annotation_required_prop: info


Is info the default severity of this?

It doesn't really have a default severity since it's disabled by default, and to enable it you also have to specify a severity

sydneyjodon-wk · 2024-01-24T19:28:09Z

tools/analyzer_plugin/playground/web/missing_required_props.dart

+  final incompleteBuilder = WithLateRequired();
+  incompleteBuilder();
+
+  WithLateRequired()();


I haven't looked at this PR, but I was just messing around in the playground and wondering if it shouldn't be linting when required prop validation is disabled on the usage like this: (or maybe I missed something)

(WithLateRequired()..disableRequiredPropValidation())();

Oh good catch, I forgot about that. Hmm. Yeah either we could not lint when that call is present, or make them also add an ignore comment when it's present.

I'm leaning toward not linting when it's present? What do you guys think? @sydneyjodon-wk @aaronlademann-wf

I also like not linting when it's present

Done in a7e3f7670f33bb530a738856f01faa23c3fdf2c5!

… for now

greglittlefield-wf · 2024-01-24T21:16:47Z

tools/analyzer_plugin/test/unit/util/prop_declaration/shared_util.dart

+      // FIXME(FED-2015) remove once these are properly ignored in generated code
+      .where((e) => e.errorCode.name.toLowerCase() != 'invalid_use_of_visible_for_overriding_member')


This was cherry-picked from this PR here: c859acb to get CI passing after merging the WIP branch, and will get addressed in that PR (FIXME removed in a later commit here 008939c)

sydneyjodon-wk · 2024-01-25T19:46:29Z

Starting QA on this :loading:

sydneyjodon-wk

QA +1 Smoke tested

Manually verify that diagnostic behaves as expected in tools/analyzer_plugin/playground/web/missing_required_props.dart
Remove the over_react_annotation_required_prop line in the playground's analysis_options.yaml and verify that annotation-required props don't lint by default
Try typing in the playground and verify there are virtually no InconsistentAnalysisExceptions errors compared to the base branch
Verify behavior of debug flag comments looks good:
- // debug: over_react_metrics
  - Verify new behavior mentioned in Changes section looks good
  - Verify the performance of this diagnostic looks good
- // debug: over_react_required_props - shows information about required props for each builder

greglittlefield-wf added 30 commits December 18, 2023 17:17

Add more efficient AnalyzerDebugHelper method

6d013bf

Reenable plugin, different codes based on late/annotation, respect ig…

0ef53e8

…nores

Add some tests for getAllProps, respect Accessor(doNotGenerate: true)

1f0f7d5

First stab at supporting legacy props classes in getAllProps

ab3657a

Optimize required prop diagnostic

8508cd4

Display the top 3 slowest diagnostics in the metrics info

609d030

Collect timings on shared getAllComponentUsages call

fe79396

Cache required prop info, optimize missing prop check

efdc268

Include source props clas in message

4399eaf

Work around InconsistentAnalysisException when computing fixes

71ea105

Fix incorrect typing

705eba9

Merge remote-tracking branch 'origin/analyzer_plugin_null_safety' int…

df59d0c

…o required-props-lint

Clean up getAllProps and its usages

87cd066

Add test cases for subclasses

11d9b6e

Add some more getAllProps test cases

abd83ce

Add some tests, fix builder variables being linted

6798983

Add tests for @requiredProp props

babd8f1

Add test for inherited props

2c0e8b3

Update playground example

3b67fa8

Add test case for potential name conflicts with prefixed props

0a85ca5

Make return types more specific so that matcher extensions can be used

3cc05a9

Assert fix missage

0ce099a

Add test for generic factory

9f086fc

Reorganize and document required prop utils

39b0b7c

Clean up required props utils, bump to SDK 2.17 to get enhanced enums

364e7fd

Rename getRequiredPropInfo to getAllRequiredProps

1a66482

Add tests for required props utils, share setup with getAllProps tests

8db7bb6

Add tests for UiProps

9e6dd9f

Add tests for renaming required prop utilities

2437c09

Clean up debug message code, share expando caching

b84374b

greglittlefield-wf requested a review from sydneyjodon-wk January 11, 2024 00:55

greglittlefield-wf commented Jan 11, 2024

View reviewed changes

greglittlefield-wf added 3 commits January 10, 2024 18:08

Add more info in _cachedGetAllRequiredProps doc comment

a5e06bc

Add some more test assertions, clean up file names

3e57d2f

Add a little more context in doc comment

1053207

greglittlefield-wf commented Jan 11, 2024

View reviewed changes

greglittlefield-wf changed the base branch from analyzer_plugin_null_safety to v5_wip January 18, 2024 21:38

greglittlefield-wf added 3 commits January 18, 2024 14:39

Merge remote-tracking branch 'origin/v5_wip' into required-props-lint

72bd506

Add regression tests for late and annotation required props colliding

f32bd6b

Handle collisions with late and annotation required props

3565e80

Fixes regression tests added in previous commit

greglittlefield-wf commented Jan 19, 2024

View reviewed changes

greglittlefield-wf added 2 commits January 22, 2024 15:00

Improve error messages when there are issues with test source

4a79c4b

Add some missing test cases

9e1c426

greglittlefield-wf mentioned this pull request Jan 22, 2024

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

Merged

7 tasks

Clean up short-circuit

2ca53e4

aaronlademann-wf approved these changes Jan 24, 2024

View reviewed changes

sydneyjodon-wk reviewed Jan 24, 2024

View reviewed changes

greglittlefield-wf added 2 commits January 24, 2024 12:52

Merge remote-tracking branch 'origin/v5_wip' into required-props-lint

c0cff8d

Don't lint for builders with disableRequiredPropValidation calls

a7e3f76

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

Ignore invalid_use_of_visible_for_overriding_member in generated code…

c82e841

… for now

greglittlefield-wf commented Jan 24, 2024

View reviewed changes

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

greglittlefield-wf mentioned this pull request Jan 24, 2024

FED-2139 Fix ProviderProps.value being optional/nullable, refactor internal impl #876

Merged

7 tasks

aaronlademann-wf approved these changes Jan 25, 2024

View reviewed changes

sydneyjodon-wk approved these changes Jan 25, 2024

View reviewed changes

greglittlefield-wf merged commit 4555104 into v5_wip Jan 25, 2024
9 checks passed

greglittlefield-wf deleted the required-props-lint branch January 25, 2024 23:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FED-1863 Required props lint #871

FED-1863 Required props lint #871

greglittlefield-wf commented Jan 11, 2024 •

edited

Loading

greglittlefield-wf Jan 11, 2024

greglittlefield-wf Jan 11, 2024

greglittlefield-wf Jan 11, 2024

greglittlefield-wf Jan 19, 2024 •

edited

Loading

aaronlademann-wf left a comment

aaronlademann-wf Jan 24, 2024

greglittlefield-wf Jan 24, 2024

aaronlademann-wf Jan 24, 2024

aaronlademann-wf Jan 24, 2024

greglittlefield-wf Jan 24, 2024

sydneyjodon-wk Jan 24, 2024 •

edited

Loading

greglittlefield-wf Jan 24, 2024

sydneyjodon-wk Jan 24, 2024

greglittlefield-wf Jan 24, 2024

greglittlefield-wf Jan 24, 2024

sydneyjodon-wk commented Jan 25, 2024

sydneyjodon-wk left a comment

		@@ -53,62 +54,121 @@ render() {
		// </editor-fold>

		class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor {

		// FIXME(FED-2015) remove once these are properly ignored in generated code
		.where((e) => e.errorCode.name.toLowerCase() != 'invalid_use_of_visible_for_overriding_member')

FED-1863 Required props lint #871

FED-1863 Required props lint #871

Conversation

greglittlefield-wf commented Jan 11, 2024 • edited Loading

Dependent PR

Motivation

Changes

Release Notes

Review

QA Checklist

Merge Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglittlefield-wf Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sydneyjodon-wk Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sydneyjodon-wk commented Jan 25, 2024

sydneyjodon-wk left a comment

Choose a reason for hiding this comment

greglittlefield-wf commented Jan 11, 2024 •

edited

Loading

greglittlefield-wf Jan 19, 2024 •

edited

Loading

sydneyjodon-wk Jan 24, 2024 •

edited

Loading