Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Committee feedback: fallback behavior #39

Closed
ljharb opened this issue Aug 25, 2018 · 22 comments
Closed

Committee feedback: fallback behavior #39

ljharb opened this issue Aug 25, 2018 · 22 comments

Comments

@ljharb
Copy link
Member

ljharb commented Aug 25, 2018

Per #38 and a few other comments, what does String.prototype.matchAll do when delete RegExp.prototype[Symbol.matchAll]?

Options:

  1. Current behavior: the same thing as if RegExp.prototype[Symbol.matchAll] remained as the original primordial value. My argument for this is here: [spec] Adjust to committee feedback #38 (comment)
  2. Instead of calling into CreateRegExpStringIterator, invoke Symbol.matchAll on the matcher regex (step 7 of https://tc39.github.io/proposal-string-matchall/#sec-string-prototype-matchall). Since it's been deleted in this scenario, it would throw a TypeError.
@littledan
Copy link
Member

littledan commented Sep 12, 2018

Looking at the behavior for @@match, @@split, @@search and @@replace, it seems like there are two templates for what the string methods do:

  1. If the argument doesn't have the symbol, then create a RegExp out of it and call the method (@@match and @@search)
  2. If the argument doesn't have the symbol, make a String out of it and run special string behavior (@@replace and @@split)

I've previously argued that we should have the special string-like behavior in 2 for matchAll, but I guess I lost that argument. Given this, I'd say we should follow 1, and throw a TypeError if you delete RegExp.prototype[Symbol.matchAll]. (Basically, I'm agreeing with @anba in #38 (comment))

@mathiasbynens
Copy link
Member

@littledan's option 1 (which somewhat confusingly amounts to @ljharb's option 2) is what I would intuit as well. It matches @@match.

@ljharb
Copy link
Member Author

ljharb commented Sep 14, 2018

I don't think that matching .match here is appropriate, because match's behavior is conflated by being both "match the regex" and "mark this thing as being a regex". Without the "mark this thing" semantic, I think match would have had this same fallback behavior.

@littledan
Copy link
Member

I don't see how marking is relevant here. Anyway, maybe we should follow @@search then? Or, if not, we could follow the other two. I just don't see the case for this middle point.

@ljharb
Copy link
Member Author

ljharb commented Sep 16, 2018

I don't think we need to pick a single method to follow to make both decisions. The committee's decision to "create a RegExp out of it" need not be coupled to the decision to work (or not) in the absence of the RegExp.prototype Symbol method.

What I still haven't heard from anyone, however, is an argument besides "it doesn't feel right" or "let's pick something to be consistent with". Is there an implementability cost to this? Does it somehow make for a confusing mental model, or does it make for a surprising and unpleasant result, for someone in this edge case world with delete RegExp.prototype[Symbol.matchAll] and string.matchAll(regex)? I'm just really not clear on who would be helped by an exception in this case.

@littledan
Copy link
Member

It's not clear to me why you consider choosing one of the two options that I listed above to be an exception. I'm not sure what kind of argument you are looking for--didn't you make a lot of these decisions out of consistency/analogy arguments?

I don't think this decision will matter much for JS developers or implementers, though (unless we follow the split/replace pattern, which would be useful).

@ljharb
Copy link
Member Author

ljharb commented Sep 16, 2018

It’s the difference between whether someone who cares about robust JS code has to cache 1 method, or 2.

@ljharb
Copy link
Member Author

ljharb commented Sep 16, 2018

Note as well that the changes expected during stage 3 are “Limited: only those deemed critical based on implementation experience”. I’m still not sure how this change is critical nor based on implementation experience.

@mathiasbynens
Copy link
Member

Note as well that the changes expected during stage 3 are “Limited: only those deemed critical based on implementation experience”. I’m still not sure how this change is critical nor based on implementation experience.

matchAll advanced to stage 3 in January, where the committee also decided on the "coerce to regexp, add global flag" semantics. The discussion in #34 (comment) as well as the resulting changes happened later, in March, when @anba was reviewing the spec while working on SpiderMonkey patches. (I've pointed this out before.)

You seem to be saying this feedback should have been given prior to stage 3, which seems impossible given that the patch that started this whole discussion was created and merged after getting stage 3: #33

There was never consensus on the "robust against delete" behavior you're defending, and I don't appreciate you repeatedly dismissing this feedback:

@ljharb
Copy link
Member Author

ljharb commented Sep 17, 2018

I’m not dismissing it; it’s on the agenda for this month’s meeting.

The proposal has been robust against Symbol deletion since day one - that was part of what received reviewer and editor approval. Here is the line for when it reached stage 2. This part isn’t new and I’ve never intended to land this feature without it.

@littledan
Copy link
Member

My approval for Stage 3 was based under the misunderstanding that we had addressed the issues @anba raised. I think we'd be able to get to Stage 4 the fastest if we could figure out how to work through those.

@ljharb
Copy link
Member Author

ljharb commented Sep 17, 2018

The issues anba raised that are referred to in this issue were brought up after stage 3. They have all been addressed, as far as i know, other than the “robust against symbol deletion” question - which as far as i know had never been brought up prior, and was part of the proposal through all previous stages, including full spec review.

Since we’ll be discussing this at this month’s meeting, and since stage 3 changes are intended to only be critical and based on implementation experience, I’d like to understand how having this particular robustness causes problems for implementations, or how it constitutes a critical problem, so that i can prepare for my presentation.

@littledan
Copy link
Member

I don't think it's been claimed that this is an implementation issue, or a critical problem. But I think, during Stage 3, we should be open to minor tweaks based on the more detailed reviews that (unfortunately) happen later in the process. For example, for class fields, we tweaked the TDZ of this during Stage 3. If our interest is getting this thing standardized and shipped to developers, I think creating a space to have this discussion openly would be the fastest way to make that happen, so I'm happy that we'll be discussing it at TC39.

@ljharb
Copy link
Member Author

ljharb commented Sep 17, 2018

Certainly; just want to understand the context.

In that case, I’d like to understand which users are helped by not having this fallback?

@littledan
Copy link
Member

I'll continue to say, this is not a major issue for JavaScript developers or implementers. This is a matter of consistency and not further complicating the already complicated RegExp object. I continue to prefer if we went with the split/replace-style model, but would accept the match/search model.

@zenparsing
Copy link
Member

Another lens through which to look at this is to say that if we throw now, we'll probably be able to change it to non-throwing later (as is perhaps possible for search).

@mathiasbynens
Copy link
Member

During the September TC39 meeting there was strong objection to @ljharb's option 1. Can we go with option 2 so we can move forward with this proposal please?

@peterwmwong
Copy link

@ljharb Will we be moving forward with option 2?

@ljharb
Copy link
Member Author

ljharb commented Nov 6, 2018

A delegate requested that we ask, instead of notify, the committee - so we can’t move forward until after the November meeting either way.

If tc39/ecma262#1318 or a similar change seems like it could proceed, then option 2 would be acceptable - we’ll have to see how the November meeting plays out.

@peterwmwong
Copy link

@ljharb Okay, Thanks for the update!

@littledan
Copy link
Member

That PR got some resistance on the thread (though I like it). What do you think we should do if it is not accepted?

@ljharb
Copy link
Member Author

ljharb commented Nov 29, 2018

tc39/ecma262#1318 has achieved consensus (pending web compatibility investigation), so I'm going to move forward with option 2, and will update the proposal repo shortly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants