Skip to content

Add a test for "regex"es that use \a (ASCII BEL) #309

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

Closed
leadpony opened this issue Nov 24, 2019 · 9 comments
Closed

Add a test for "regex"es that use \a (ASCII BEL) #309

leadpony opened this issue Nov 24, 2019 · 9 comments
Labels
missing test A request to add a test to the suite that is currently not covered elsewhere.

Comments

@leadpony
Copy link
Contributor

Hello
The 4th and 5th test cases defined in ecmascript-regex.json use the following pattern keyword in the input schema.

"pattern": "^\\a$"

The ECMA-262 specification does not allow a as a letter for control escape in its syntax definition.

ControlEscape :: one of 
  f n r t v

I tested it with Node.js as follows:

node
Welcome to Node.js v12.13.1.
Type ".help" for more information.
> var re = new RegExp('^\\a$');
undefined
> re.test('\u0007');
false

The test case in ecmascript-regex.json seems incompatible with the test() result above.
How should we treat these test cases?

@Julian
Copy link
Member

Julian commented Nov 24, 2019

Seems you're right to me from what I can tell.

@Zac-HD might be able to weigh in having presumably just read through that spec, but assuming you're right here, probably worth changing that one to use one of the other escape characters, and then explicitly adding one saying \a isn't valid.

@Julian Julian added the bug A test is wrong, or tooling is broken or buggy. label Nov 24, 2019
@leadpony
Copy link
Contributor Author

I would like to hear the initial intention of the test from @Zac-HD if possible.
I would rather not break it.

@Zac-HD
Copy link
Contributor

Zac-HD commented Nov 25, 2019

We should switch as @Julian suggests; I was just more familiar with the Python than the ECMA string escapes.

@leadpony
Copy link
Contributor Author

leadpony commented Nov 25, 2019

@Julian @Zac-HD
Thank you for your quick responses.
Switching \\a to another escape such as \\t is OK, however, leaving test with \\a may create another problem.

There exist 2 regular expression patterns defined in the ECMA-262 specification.

  1. 21.2 RegExp (Regular Expression) Objects
  2. B.1.4 Regular Expressions Patterns

Note that the latter pattern is a part of B. Additional ECMAScript Features for Web Browsers.

Both patterns do not allow a as a controll escape letter, but the final consequence seems to slightly differ.
In the latter pattern, \\a is converted to a letter a, as Node.js does, because the letter is recognized as a part of IdentifyEscape.
In the former pattern, I believe a syntax error should be thrown, because a has an ID_CONTINUE Unicode property and cannot be handled as an IdentifyEscape.

IdentityEscape::
  SourceCharacter but not UnicodeIDContinue

The JSON Schema Specification does not state which pattern should be followed, therefore we cannot make the test without some ambiguity.

@Julian
Copy link
Member

Julian commented Nov 27, 2019

Thanks for doing the research. Seems like probably something to raise upstream on the spec if there's missing clarity then yeah.

@Julian
Copy link
Member

Julian commented Nov 27, 2019

(At which point we can probably just decouple the two of these things -- immediately change the \a in a PR, and then just file an issue to add a test for \a's behavior one way or the other as soon as the spec clarifies it)

Sound reasonable?

leadpony added a commit to leadpony/JSON-Schema-Test-Suite that referenced this issue Nov 27, 2019
At least this fixes the wrong test case reported in issue json-schema-org#309
@Julian Julian changed the title Bell code problem in ecmascript-regex.json Add a test for "regex"es that use \a (ASCII BEL) Nov 29, 2019
@Julian
Copy link
Member

Julian commented Nov 29, 2019

Filed json-schema-org/json-schema-spec#821 -- we can leave this ticket open until the upstream issue is clarified, and then add a test for the decided upon behavior.

@Julian Julian added missing test A request to add a test to the suite that is currently not covered elsewhere. and removed bug A test is wrong, or tooling is broken or buggy. labels Nov 29, 2019
@Julian Julian closed this as completed in e42e841 Jun 15, 2022
@ChALkeR
Copy link
Member

ChALkeR commented Jul 3, 2022

@Julian e42e841 introduces a schema that's invalid afaik, and the test expects that schema to fail validation on input it receives, which is different.

I think that the correct behavior would be to refuse to compile (or load) this schema at all, not just fail everything in runtime.
That's not what that test is checking.

Upd: ah, ignore me, I misread, that's in data, not schema! Sorry for the noise.

@karenetheridge
Copy link
Member

For more complete coverage, you could test "format": "regex" against the data instance "\a".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
missing test A request to add a test to the suite that is currently not covered elsewhere.
Projects
None yet
Development

No branches or pull requests

5 participants