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

GetOptionsObject vs OrdinaryObjectCreate(%Object.prototype%) #2098

Closed
Tracked by #1424
Ms2ger opened this issue Mar 16, 2022 · 9 comments · Fixed by #2219
Closed
Tracked by #1424

GetOptionsObject vs OrdinaryObjectCreate(%Object.prototype%) #2098

Ms2ger opened this issue Mar 16, 2022 · 9 comments · Fixed by #2219
Assignees
Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Milestone

Comments

@Ms2ger
Copy link
Collaborator

Ms2ger commented Mar 16, 2022

NormalizeOptionsObject is used in some places when an options argument is needed, which creates a new object inheriting from null instead of Object.prototype. A bunch of other places instead do OrdinaryObjectCreate(%Object.prototype%). It seems like these should be consistent, and (as a non-editorial opinion) NormalizeOptionsObject seems better: I wouldn't expect omitting an options argument to cause lookups on Object.prototype.

Some earlier discussion in #1424

@ljharb

This comment was marked as resolved.

@Ms2ger Ms2ger changed the title NormalizeOptionsObject is used in some places when an options argument is needed, which creates a new object inheriting from null instead of Object.prototype. A bunch of other places instead do OrdinaryObjectCreate(%Object.prototype%). It seems like these should be consistent, and (as a non-editorial opinion) NormalizeOptionsObject seems better: I wouldn't expect omitting an options argument to cause lookups on Object.prototype. NormalizeOptionsObject vs OrdinaryObjectCreate(%Object.prototype%) Mar 16, 2022
@Ms2ger

This comment was marked as outdated.

@Ms2ger Ms2ger added the spec-text Specification text involved label Mar 16, 2022
@Ms2ger Ms2ger added this to the Next milestone Mar 16, 2022
@ljharb

This comment was marked as outdated.

@ptomato
Copy link
Collaborator

ptomato commented Mar 16, 2022

I thought this was fixed already; are there any places left where an options object created within Temporal does not have a null prototype?

@linusg
Copy link
Member

linusg commented Mar 16, 2022

Note that NormalizeOptionsObject is now called GetOptionsObject (see a7458d3); you might want to update the issue title & description.

@Ms2ger Ms2ger changed the title NormalizeOptionsObject vs OrdinaryObjectCreate(%Object.prototype%) GetOptionsObject vs OrdinaryObjectCreate(%Object.prototype%) Mar 17, 2022
@Ms2ger
Copy link
Collaborator Author

Ms2ger commented Mar 17, 2022

I found:

  • Temporal.PlainYearMonth.prototype.add/subtract
  • DefaultMergeFields / Temporal.Calendar.prototype.mergeFields
  • MergeLargestUnitOption
  • PrepareTemporalFields
  • PreparePartialTemporalFields

Plus getISOFields, DateTimeFormat, and the Temporal objects themselves, but those seem fine.

@ptomato
Copy link
Collaborator

ptomato commented Mar 17, 2022

I agree PlainYearMonth.add/subtract and MergeLargestUnitOption should be fixed.

mergeFields/DefaultMergeFields and PrepareTemporalFields/PreparePartialTemporalFields I didn't consider before because they weren't options objects. But maybe it makes sense to make these into null prototype objects too. For objects returned from PrepareTemporalFields it really shouldn't matter because we only Get properties that are guaranteed to be own data properties of the object. But for PreparePartialTemporalFields, you could write some evil code where it is observable:

Temporal.PlainDate.from('2022-01-01').with({
  get year() {
    Object.prototype.day = 31;
    return 2023;
  },
});

You'd expect this to result in 2021-01-01 but it might result in 2021-01-31...

@ptomato ptomato added the normative Would be a normative change to the proposal label May 6, 2022
@Ms2ger
Copy link
Collaborator Author

Ms2ger commented May 18, 2022

I'd say:

  • Let's change PlainYearMonth.add/subtract
  • I think mergeFields is fine with Object.prototype since it just returns the object back to the caller
  • Change MergeLargestUnitOption
  • Seems like PrepareTemporalFields doesn't matter
  • Change PreparePartialTemporalFields as well

@ptomato
Copy link
Collaborator

ptomato commented May 18, 2022

Sounds like we agree. If PrepareTemporalFields doesn't matter, then probably we should still change it for consistency with PreparePartialTemporalFields, since consistency is the reason the issue was opened in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants