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.

Sign up for GitHub

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

Jump to bottom

_UnmodifiableMapMixin.remove should return Object? – right? #44264

Closed
kevmoo opened this issue Nov 20, 2020 · 5 comments
Closed

_UnmodifiableMapMixin.remove should return Object? – right? #44264

kevmoo opened this issue Nov 20, 2020 · 5 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-collection NNBD Issues related to NNBD Release P1 A high priority bug; for example, a single project is unusable or has many test failures type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@kevmoo
Copy link
Member

kevmoo commented Nov 20, 2020

sdk/sdk/lib/collection/maps.dart

Lines 287 to 290 in 818b665

/// This operation is not supported by an unmodifiable map.
V remove(Object? key) {
throw UnsupportedError("Cannot modify unmodifiable map");
}

The return signature should be Object? – right?

Inconsistent with other impls right now
@kevmoo kevmoo added P1 A high priority bug; for example, a single project is unusable or has many test failures area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-collection NNBD Issues related to NNBD Release labels Nov 20, 2020
@kevmoo
Copy link
Member Author

kevmoo commented Nov 20, 2020

CC @lrhn

@lrhn
Copy link
Member

lrhn commented Nov 20, 2020 •
edited

It should be V? to match the other implementations.
Since it always throws, it's not wrong to claim to return a narrower set of values (using Never as return type would be correct).

No-one should ever be using the type _UnmodifiableMapMixin (or seeing it anywhere), so it doesn't actually matter what the declared return type is, but it can affect classes like UnmodifiableMapView too.

So, it's slightly misleading as it is, but not wrong.
We can change it to V? to pretend it's like other maps, or we can change it, and all the other "guranteed throwing" methods to return Never. That might actually be useful. If you ever have something statically typed as UnmodifiableMapView, you won't be able to use .remove without realizing that it throws.

The biggest issue is that someone might extend an "unmodifiable" type with a method which doesn't throw. Such abuse would break. I'd like that 😁 .

@lrhn lrhn added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Nov 20, 2020
@kevmoo
Copy link
Member Author

kevmoo commented Nov 20, 2020 via email

@lrhn
Copy link
Member

lrhn commented Nov 20, 2020

Let's try how changing them to Never works out: https://dart-review.googlesource.com/c/sdk/+/173266

@kevmoo
Copy link
Member Author

kevmoo commented Nov 24, 2020

Could we do the simple fix? I'd love to get this in ASAP – one of those weird corner things I'd rather not hit later!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-collection NNBD Issues related to NNBD Release P1 A high priority bug; for example, a single project is unusable or has many test failures type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@kevmoo @lrhn