Skip to content
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

Simplify InheritingContainer.allModelElements and _inheritedElements #3689

Merged
merged 2 commits into from
Feb 28, 2024

Conversation

srawlins
Copy link
Member

@srawlins srawlins commented Feb 28, 2024

I could not wrap my head around the interactions between InheritingContainer._allModelElements, which seemed to fill a cache, then do work, then clear the cache, and InheritingContainer._inheritedElements. _inheritedElements and _inheritedElementsCache seemed like a clear instance of a late final field before null safety (before late), but then the cache invalidation in _allModelElements kind of nullifies that theory...

Anyways, it can all be simpler:

  • _allModelElements can just be the list.
  • _inheritedElementsCache is no more; _inheritedElements is now a late final.
  • I cannot fathom why inheritanceChainElements was initialized in the for loop... so its now initialized outside the for loop. 🤷
  • Iterating over someMap.keys and immediately calling back into the map and null-asserting the result is too much work. We can instead iterate over the entries (hopefully performant...) and destructure the result.
  • Add more comments for the next archaeologist.

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

/// _this_ element are preferred over elements that are further away. In the
/// case of ties, concrete inherited elements are prefered to non-concrete
/// ones.
late final List<ExecutableElement> _inheritedElements = () {
if (element is ClassElement && (element as ClassElement).isDartCoreObject) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    if (element case ClassElement classElement
        when classElement.isDartCoreObject) {
      return const <ExecutableElement>[];
    }

Maybe? Instead of casting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it

// [InheritanceManager3.getInheritedConcreteMap2] is farther from this
// class in the inheritance chain than the one provided by
// If the concrete object from `getInheritedConcreteMap2` is farther in
// the inheritance chain from this class than the one provided by
// `inheritedMap2`, prefer `inheritedMap2`. This correctly accounts for
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is inheritedMap2? Is this comment outdated or am I missing something?
Or does this just mean inheritanceMap and "the one provided" == inheritedElement?
Maybe fix the comments to be a tad more clear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, wrong name. I am referring to the getInheritedMap2 method. Updated.

packageGraph.inheritanceManager.getInheritedMap2(element);

List<InterfaceElement>? inheritanceChainElements;
var inheritanceChainElements =
inheritanceChain.map((c) => c.element).toList(growable: false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also these three maps above are named so similarly. Can we add a comment on what the difference is between them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, the doc comment on inheritanceChain only describes what it is not ("Not the same as [superChain] as it may include mixins."). OK so I've updated that comment. And some more. And then introduced some comments above these declarations.

I cannot think of better names; they're already long and I think making them longer would hurt readability. But I'm open to new name ideas.

FWIW I don't know why this algorithm was chosen for _inheritedElements, but I'm strongly preferring to leave it alone; it is likely depended on by Flutter. It might be documented in tests.

@srawlins srawlins merged commit eed92d3 into dart-lang:main Feb 28, 2024
9 checks passed
@srawlins srawlins deleted the inherited-model-elements branch February 28, 2024 17:06
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Feb 28, 2024
Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/40c470e..eed92d3):
  eed92d3f  2024-02-28  Sam Rawlins  Simplify InheritingContainer.allModelElements and _inheritedElements (dart-lang/dartdoc#3689)
  7b3274a6  2024-02-28  Sam Rawlins  Do not require FLUTTER_ROOT any longer, even for flutter packages (dart-lang/dartdoc#3688)
  d25dfca4  2024-02-27  Kallen Tu  Remove LanguageFeatureRenderer. (dart-lang/dartdoc#3686)
  d8e7f99c  2024-02-27  Sam Rawlins  Display the immediate representation type, not any erasure (dart-lang/dartdoc#3685)
  91c361af  2024-02-27  Kallen Tu  Remove SourceCodeRenderer and TypeParametersRenderer. (dart-lang/dartdoc#3683)

markdown (https://github.com/dart-lang/markdown/compare/d735b0b..62e3349):
  62e3349  2024-02-27  Tom Yeh  Fix `#586`: encode image tag's src attribute (dart-lang/markdown#589)

mockito (https://github.com/dart-lang/mockito/compare/7d6632f..3ef744f):
  3ef744f  2024-02-27  Ilya Yanok  Bump SDK version using in CI to 3.3
  ecec7c1  2024-02-01  dependabot[bot]  Bump dart-lang/setup-dart from 1.6.0 to 1.6.2
  b693ada  2024-02-27  Ilya Yanok  Add basic extension types support

test (https://github.com/dart-lang/test/compare/a3f05ec..26953ba):
  26953ba4  2024-02-26  Sam Rawlins  Update README.md, remove link to Stream Matchers (dart-lang/test#2187)

tools (https://github.com/dart-lang/tools/compare/c7fbf26..fca993e):
  fca993e  2024-02-28  Elias Yishak  Fix late initialization error for `Analytics.userProperty` (dart-lang/tools#239)
  1a0d7da  2024-02-26  Elias Yishak  Export `testing.dart` with all enums (dart-lang/tools#237)

web (https://github.com/dart-lang/web/compare/d96c01d..fa4280c):
  fa4280c  2024-02-27  Devon Carew  mention the migration guide in the readme (dart-lang/web#187)

Change-Id: Ib183b4af55146fdeabfb18dc91bf642159e67a1b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/354963
Reviewed-by: Kevin Moore <kevmoo@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants