-
Notifications
You must be signed in to change notification settings - Fork 21
Why throw SyntaxErrors in 8.1 Runtime Semantics: GetSubstitution #29
Comments
These ideas are reasonable and consistent with other ways the replace string works, but I'm not sure if we should switch to them. The current semantics were chosen based on a general design principle that, for new language features, we should be stricter and throw exceptions where it's reasonable. This is malformed syntax. Another example of something similar is how Unicode RegExps don't allow invalid escape sequences to just be ignored (e.g., FWIW these aren't the only way exceptions can be thrown from this code path. WIth RegExp subclassing or monkey-patching, a groups object may be provided which doesn't behave appropriately, and leads exceptions to be thrown (either directly or from the ToString on the result). I don't see a good way to ignore this; it would be pretty extreme to actually catch and suppress these errors. Any more thoughts? @schuay @hashseed @mathiasbynens |
We discussed part of your suggestion this bug thread: #14 |
It is clear from @littledan's reply and from bug thread #14, that there isn't an overwhelming drive for throwing a SyntaxError in the two cases. First off, I would contend that SyntaxError is wrong for Furthermore, a SyntaxError or possibly the more suitable TypeError does not convey sufficient information for reasonable code to determine what exactly is wrong with the replacement string. Having defined semantics for both the missing closing '>' and a properly formed group name that isn't present in the RegExp, would make be easier for interested programmers to validate proper operation of Lastly, I can accept that new features should throw errors when extending existing methods where appropriate. In this case though, the proposal describes throws a SyntaxError during the runtime processing of a String utility method. All other existing String methods throw either Range or Type errors. In each of those cases, the error is detected and thrown early in the processing of the method and with a clear reason for the error. This proposed SyntaxError is thrown during processing and only catches the first of what could be several errors and provides very little direct indication as to the cause. As an implementor who just finished the implementation of named capture groups in JavaScriptCore, my feedback is that it is troubling that part way through constructing the result we'd throw an error. In existing code that calls The code currently checked into JavaScriptCore implements the changes I propose. That code will be updated as appropriate based on the resolution of this issue. |
I agree; this is a sort of matter of taste.
FWIW JSON.parse also throws SyntaxError... I'm not sure why TypeError makes all that much sense, since nothing is of the wrong type.
No standardized errors give you enough information to debug. It's the job of engines to give usable error messages. Do you think it would be especially impractical to do so in this case?
I think this is the strongest argument, and a very important constraint. @jgruber, did your implementation here of named groups use your RegExp subclassing fastpath? Would the proposed changes make it easier to do so? |
+1, and an exception gives more information than just failing silently.
No, this makes no difference to our implementation in V8.
I still don't have a strong opinion either way. If there are good arguments for silent failure vs. throwing, then I'm happy to go with that. |
@msaboff Would throwing an exception in the middle of processing pose more of a challenge to throw an error partway through for issues in the replacement string, than the Get for what the actual named capture is? Some possible alternative semantics, if we want all the exceptions thrown upfront:
That preserves the current semantics of more actively giving users errors, while moving all exceptions to the beginning of the replacement. Thoughts? |
-1 from me for upfront scanning & named group collection, seems like that would complicate things unnecessarily. I'm not sure I understand what the issues are with throwing during result construction, @msaboff could you perhaps elaborate on that? |
It seems to me that TypeError is more consistent with existing uses elsewhere.
I said nothing about failing silently. My suggestion is that we define semantics for all forms of
The same is true about JavaScriptCore. I don't not advocate a 2 pass solution to the problem. That would impact performance. |
Could you give an example? |
I concur. I don't want upfront scanning for error and then replacement processing, i.e. two passes over the replacement string.
My main argument is that we are proposing a change in the operation of Consider existing code with calls to |
Is it at all a problem for language design or JSC's implementation that the Get from the groups object may throw, observably in the middle of the replacement? This is also a new error path that didn't exist previously. I get the point about how there were previously no syntax errors, and now there are, but Unicode RegExps made an analogous change.
The breakage would only happen if you use it with a new RegExp with named groups. So, this can break existing libraries, but it can't break existing deployed websites. Even with your proposed fix, if an existing replacement string contained |
In 19.5.5.4 SyntaxError, it states:
In 19.5.5.5 TypeError is states:
In almost all cases, SyntaxError is thrown for malformed Java Script or JSON source, malformed RegExp expression, failed parameter sanitization, and strict mode errors. In 8.1 Runtime Semantics: GetSubstitution the steps describe copy with replacement of $ prefixed strings. This isn't a hill for me to die on though as I don't think we should be throwing any error. |
No, throwing from Get isn't an issue. The difference is that exceptions along Get and other property access paths are not new and well understood. My issue is the direct introduction of throwing an error in an algorithm that didn't have throws for any of the existing cases that also handle malformed |
+cc @ajklein |
Now that implementations are starting to ship, let’s get this issue resolved. I’m okay with making the proposed change. |
@msaboff's arguments sound good to me, I'd be happy with this spec change (and as @mathiasbynens says, I'd like to get this resolved so we can move forward with un-flagging V8's implementation). |
+1 from me as well. |
Also +1. Let's implement this and ship! |
The specced semantics of GetSubstitution are expected to change in the case of malformed named references, or named references to nonexistent named groups. The former will evaluate to the identity replacement of '$<', while the latter will result in replacement by the empty string. See also: tc39/proposal-regexp-named-groups#29 Bug: v8:5437, v8:6912 Cq-Include-Trybots: master.tryserver.v8:v8_linux_noi18n_rel_ng Change-Id: I879288f775774cb0ec563f9d9129a99710efb77c Reviewed-on: https://chromium-review.googlesource.com/708654 Commit-Queue: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{#48426}
OK, glad we could resolve this. I'll fix up the spec and test262 tests soon to reflect the V8/JSC consensus of not throwing errors in these cases. |
OK, committed a patch to attempt to match the implementations; will update tests soon. cc @ljharb |
In Table 1: Replacement Text Symbol Substitutions,
SyntaxErrors
are thrown in two situations. The first in the case that the$<groupName
is not terminated by a closing'>'
. The second is when the groupName is not a named group in the regular expression. This introduces the runtime first exceptions for the non-function flavor ofString.prototype.replace
.It seems that both cases can be handled by defining alternative semantics. For the missing closing
'>'
, treat the$<groupName
as the replacement.In the second case, use
empty string
as the replacement.This would eliminate the need for programmers to wrap calls with try / catch for proper coding.
The text was updated successfully, but these errors were encountered: