Skip to content

Cast JSON objects to their expected type avoiding follow-on dyamic calls#6948

Closed
srawlins wants to merge 1 commit intomasterfrom
dynamic-refactor-2
Closed

Cast JSON objects to their expected type avoiding follow-on dyamic calls#6948
srawlins wants to merge 1 commit intomasterfrom
dynamic-refactor-2

Conversation

@srawlins
Copy link
Contributor

Work towards #6929. This is the big one.

This diff is unpleasant, with so many explicit casts. But this is what is required to avoid dynamic calls in this JSON-processing-heavy code. Feedback about whether we want dynamic-free, explicit cast code, as a requirement (like, guarded with a lint rule), is 100% welcome please let me know 😁 .

This diff is not too large (+75/-72 lines) but it touches a lot of files, and I'm happy to break it up, if that helps the review.

The new extension types should be a real alternative to casting JSON-parsed objects like this. They require Dart 3.3, so we can't land any yet (I think?). But as an example, let's look at the diff in logging_controller.dart. Instead of casting each value like entry['source'] as String?, we can type entry as an Extension Type called Entry:

extension type Entry(Map<String, Object?> data) {
  String? get source => data['source'];
  Map<String, Object?>? get displaySize => (data['displaySize'] as Map).cast<String, Object?>();
  Map<String, Object?>? get imageSize => (data['imageSize'] as Map).cast<String, Object?>();
}

Then all access on this object is restricted to the API declared above. If folks are OK with the code as is, and want to explore extension types later (soon), we could also kick the can down the road.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or there is a reason for not adding tests.

build.yaml badge

If you need help, consider asking for help on Discord.

@kenzieschmoll
Copy link
Member

They require Dart 3.3, so we can't land any yet (I think?)

We can use these in the devtools_app and devtools_test packages. The flutter-candidate.txt file contains the Flutter version that the DevTools CI runs with.

@srawlins
Copy link
Contributor Author

Oh cool! WDYT of extension types? I could send a proof-of-concept PR.

@kenzieschmoll
Copy link
Member

Oh cool! WDYT of extension types? I could send a proof-of-concept PR.

Let's give it a go. I'm eager to see how these work in practice!

srawlins added a commit that referenced this pull request Dec 18, 2023
An alternative to some of the code in #6948

Work towards #6929

This one is more interesting! The diff may be hard to read though. :/

* First, add `_LogRecord` to handle all JSON  access inside `_handleDeveloperLogEvent`.
* `FrameInfo` was an existing class that plainly extracted fields from JSON. We convert it to `_FrameInfo` (all access was previously from this library), an extension type.
* Also `ImageSizesForFrame` was an existing class that plainly extracted fields from JSON. In converting this to `_ImageSizesForFrame`, an extension type, you can see the casts and null-assertions getting more tidy, isolated to the extension type in charge of extracting the data from the JSON structure.
* Since the JSON is nested, we also add another extension type, `_ImageSize` to handle the nesting cleanly.
srawlins added a commit that referenced this pull request Dec 19, 2023
An alternative to some of the code in #6948

Work towards #6929

This one is again fairly simple. `CpuProfileData.parse` used to handle all of the parsing as statements, and relies on a fair amount of dynamic calling. When we extract the field calls into extension type getters, the casting etc is isolated to that section of the code. The statements in `CpuProfileData.parse` get simpler.

This could probably be improved more; I think `CpuProfileMetaData()` should just accept a `_CpuProfileDataJson` and get the data it needs. But that can be for later. Or now. Also all of the null-aware code could also be moved into the extension type. Depending on the business logic. Is it ever important to know if `sampleCount` is `null`? Or can we default it to `0` at the root? Etc.
@kenzieschmoll
Copy link
Member

@srawlins are you still planning on landing this? Or has the work here been landed in other PRs?

This diff is unpleasant, with so many explicit casts. But this is what is
required to avoid dynamic calls in this JSON-processing-heavy code.
@srawlins
Copy link
Contributor Author

It looks like basically everything in this PR landed in other PRs, as extension types and other fixes. Closing, thanks!!

@srawlins srawlins closed this Feb 13, 2024
@parlough parlough deleted the dynamic-refactor-2 branch February 14, 2024 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants