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

Tighten guidelines around dynamic #6286

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

natebosch
Copy link
Member

As the language and ecosystem has grown the target for static safety has
moved up. Previously it was considered enough to be explicit about the
places where dynamic is introduced, and then it is OK to silently do
dynamic things with it, and write code that keeps references dynamic
throughout. It is more common today to want to be explicit about places
where dynamic is used, not just where it is introduced. We can see
this in expanded use of stricter static checking and the
avoid_dynamic_calls lint.

Rephrase two guidelines to move towards being strict about using
dynamic.

  • Recommend annotating with Object? instead of dynamic when
    inferences fails. Update the code sample to use Object? in the
    method arguments are return types for the JSON related API. Update the
    text to explain how Object? is treated differently and more strictly
    statically, which makes the unsafe operations visible in syntax.
  • Rephrase from using dynamic to "disable static checking" to using it
    to "invoke dynamic members". The wording for the rule already focused
    on using dynamic to "permit all operations". Remove the paragraph
    about retaining the dynamic type when working with APIs that use it,
    instead recommending to move away from dynamic at those boundaries
    and using Object? in most code.

As the language and ecosystem has grown the target for static safety has
moved up. Previously it was considered enough to be explicit about the
places where `dynamic` is introduced, and then it is OK to silently do
dynamic things with it, and write code that keeps references `dynamic`
throughout. It is more common today to want to be explicit about places
where `dynamic` is used, not just where it is introduced. We can see
this in expanded use of stricter static checking and the
`avoid_dynamic_calls` lint.

Rephrase two guidelines to move towards being strict about using
dynamic.
- Recommend annotating with `Object?` instead of `dynamic` when
  inferences fails. Update the code sample to use `Object?` in the
  method arguments are return types for the JSON related API. Update the
  text to explain how `Object?` is treated differently and more strictly
  statically, which makes the unsafe operations visible in syntax.
- Rephrase from using dynamic to "disable static checking" to using it
  to "invoke dynamic members". The wording for the rule already focused
  on using dynamic to "permit all operations". Remove the paragraph
  about retaining the `dynamic` type when working with APIs that use it,
  instead recommending to move away from `dynamic` at those boundaries
  and using `Object?` in most code.
@dart-github-bot
Copy link
Collaborator

dart-github-bot commented Dec 19, 2024

Visit the preview URL for this PR (updated for commit 5339165):

https://dart-dev--pr6286-dynamic-guidelines-3m4bvtrq.web.app

@natebosch
Copy link
Member Author

natebosch commented Dec 19, 2024

I'm not sure how we handle a change in recommendations that impacts a link header... Do we just allow old links to break?

Edit: I'll fix existing local links if we decide to go forward with rewording these headers.

@natebosch
Copy link
Member Author

natebosch commented Dec 19, 2024

Opening this now so folks can start considering it, but I expect we won't be able to organize all the discussion around it until after the holidays.

cc @munificent what do you think?

I think this change is a real shift in the ecosystem and I'm being descriptive here, but I do have a bias and I'd want to be prescriptive about this if were writing this doc with that intent. cc @lrhn who might have a preference to keep the older style which is more permissive.

@parlough
Copy link
Member

I'm not sure how we handle a change in recommendations that impacts a link header... Do we just allow old links to break?

Since these headers usually last a long time and might be linked to from outside the site, if there's an similar or related header, I tend to add an empty anchor with the old fragment above that.

For example:

<a id="do-annotate-with-dynamic-instead-of-letting-inference-fail" aria-hidden="true"></a>

### DO annotate with `Object?` instead of letting inference fail

Copy link
Member

@lrhn lrhn left a comment

Choose a reason for hiding this comment

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

I'm fine with recommending that you don't write dynamic.

Might want to say how you then opt back into dynamic behavior. I expect it's (… as dynamic).operation() as ExpectedType.

to a more precise type before accessing members.
Prefer using `Object?` over `dynamic` in code that is not invoking a member
dynamically, even when working with existing APIs that use `dynamic`.
For example, JSON objects have runtime type `Map<String, dynamic>`, which is an
Copy link
Member

Choose a reason for hiding this comment

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

I'd say "JSON objects traditionally have the static type Map<String, dynamic>"

The runtime type doesn't matter, there is no meaningful difference between dynamic and Object? in a runtime type.

Also "JSON objects" is ambiguous. It can either mean any object created by jsonDecode, which is typed as just dynamic, or it can mean a Dart representation of a "JSON Object" (based on a JavaScript Object), which is indeed traditionally represented by a map with static type Map<String, dynamic>.
The dart: convert code doesn't mandate that you use Map<String, dynamic> instead of Map<String, Object?> when you match or cast to a Map type, that's entirely user habit. It's this just saying that you should cast those to ... Object?... before using 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.

I dropped the references to JSON objects and I'm referring just to an example of Map<String, dynamic> more generally.

dynamically, even when working with existing APIs that use `dynamic`.
For example, JSON objects have runtime type `Map<String, dynamic>`, which is an
equivalent type to `Map<String, Object?>`.
When using a value from one of these APIs, it's often a good idea to treat it as
Copy link
Member

Choose a reason for hiding this comment

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

Which APIs is this? (First use of "API" here, but the definite form suggests that it refers to something mentioned earlier.)

Is it something that expects a Map<String, dynamic>, or which produces it?
Could the previous paragraph start with
"APIs which accept or produce JSON-object maps traditionally type those as Map<String, dynamic> ..."

Copy link
Member Author

Choose a reason for hiding this comment

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

"these APIs" refers back to "existing APIS that use dynamic", not limited to the JSON example mentioned in between.

I took another try at rewriting this paragraph, and I added an example of using the (foo as dynamic).member syntax.

Copy link
Member

@munificent munificent left a comment

Choose a reason for hiding this comment

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

Overall, I really like the direction of this. I'd like to iterate a bit on the first rule though, to make it clear that it's about avoiding inference yielding a default type. (For example, if we changed inference to give you Object? when it didn't have any other type, I'd still want to have this guideline.)

When `dynamic` is the type you want, write that explicitly to make your intent
clear and highlight that this code has less static safety.
Use `Object?` to indicate in a signature that any type of object, or null, is
allowed.
Copy link
Member

Choose a reason for hiding this comment

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

The overall metarule for the guidelines is "unless stated otherwise, express things the most terse way". If you want some return type or parameter type to have type dynamic, the most terse way is to simply write nothing.

The original intent of this guideline was to opt out of that default "use the shortest thing" rule. That's why the guideline is about "instead of letting inference fail".

The new guideline here is really about "when should you use Object? versus dynamic, which is a different thing. I think the second updated guideline below does a pretty good job of covering that.

So for this one, I think it would be better to recast it to something like `DON'T let inference fall back to dynamic".

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.

5 participants