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

More RFC 8259 nits #68

Closed
1 task done
hildjj opened this issue Dec 2, 2024 · 12 comments
Closed
1 task done

More RFC 8259 nits #68

hildjj opened this issue Dec 2, 2024 · 12 comments

Comments

@hildjj
Copy link
Contributor

hildjj commented Dec 2, 2024

Rule details

Check as many of the constraints from RFC 8259 marked "interoperable in the sense"

What type of rule is this?

Warns about a potential problem

Example code

- Invalid top-level: `"foo"`
- Outside float64 (will parse as 0): `[1e-325]`
- Subnormal warning: `[4.94e-324]` 
- Too-large integer: `[18014398509481985]`
- Duplicate key (just needs a test): `{"a\\b": 0, "a\u005Cb": 1}`

Participation

  • I am willing to submit a pull request to implement this rule.

Additional comments

RFC 8259 marks several potential interoperability issues with "interoperable in the sense":

  • Only an object or array at the top level. This is the most controversial of these.
  • Unique keys (already handled by no-duplicate-keys)
  • Not order-dependent (not lintable, unless we wanted there to be another rule that enforced order, I suppose)
  • (Implied in 8259) Floats should fit into a double. Support for sub-normals should likely be optional.
  • Integers in the range in the range [-(253)+1, (253)-1]. Note that no-unsafe-values currently will not catch integers outside that range that have not gotten all the way to being +/-Infinity.
  • Valid Unicode. no-unsafe-values catches this now.
  • No duplicate keys, including ones that differ by only Unicode escapes. no-duplicate-keys only needs some tests added to check that "a\b" and "a\u005Cb" are duplicates.
@hildjj hildjj added the feature label Dec 2, 2024
@nzakas
Copy link
Member

nzakas commented Dec 2, 2024

Thanks for the feedback. I'm unclear on what it is you're suggesting. Are you saying we should update some existing rules, create new rules, etc.? Can you be more specific?

@nzakas
Copy link
Member

nzakas commented Dec 2, 2024

Also, can you please add a link to the RFC for easy reference?

@hildjj
Copy link
Contributor Author

hildjj commented Dec 2, 2024

RFC8259 is the current IETF description of JSON. It is aligned with ECMA-404, but focuses more on interoperability between systems rather than on how JSON.parse is implemented in ECMAscript/JavaScript.

Here are more explicit recommendations:

  • new rule: top-level-composite, which checks that the top-level thing in a JSON file is either an object or an array. This might be off in the default config.
  • no-unsafe-values should check for integers out of range, floats out of range, and warn on subnormals (by default).
  • Add a test for escaped characters in no-duplicate-keys.test.js

Sorry I wasn't more clear before.

@nzakas
Copy link
Member

nzakas commented Dec 2, 2024

Got it, thanks. These all seem like good additions to me.

Only question is the naming of top-level-composite, as I'm not sure it communicates the intent. Maybe interopable-top-level instead?

@nzakas nzakas added this to Triage Dec 2, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Dec 2, 2024
@nzakas nzakas moved this from Needs Triage to Ready to Implement in Triage Dec 2, 2024
@nzakas nzakas added the accepted label Dec 2, 2024
@hildjj
Copy link
Contributor Author

hildjj commented Dec 2, 2024

interopable-top-level works fine for me. Would you like me to put together a PR?

@nzakas
Copy link
Member

nzakas commented Dec 3, 2024

Or maybe top-level-interop is better? (Shorter, maybe more readable?)

And yes, but if you could please do a separate PR for each task, it would be appreciated.

@hildjj
Copy link
Contributor Author

hildjj commented Dec 3, 2024

I'm making progress on a PR.

What do we think about -0?
Safe for interop?

hildjj added a commit to hildjj/eslint-json that referenced this issue Dec 3, 2024
hildjj added a commit to hildjj/eslint-json that referenced this issue Dec 3, 2024
A new rule that checks that the top-level item in
a JSON file is either an array or an object, as
suggested by RFC8259.  Not turned on in the recommended
config because the software that had this issue
is almost entirely obsolete.

Refs eslint#68.
hildjj added a commit to hildjj/eslint-json that referenced this issue Dec 3, 2024
Note that this is affected by
humanwhocodes/momoa#164
in the sense that in JSON5 mode, unquoted escaped
keys are not yet unescaped in the same was as
all other keys, and will not trigger this lint
yet.  When the momoa issue is fixed, released, and
we have updated to that version, uncomment the
last test in no-duplicate-keys.test.js.

Suggest that we create a separate issue in this
repo to track that change.

Refs eslint#68
hildjj added a commit to hildjj/eslint-json that referenced this issue Dec 3, 2024
Flag subnormals, numbers that are flushed to zero,
and overly-large integers.

Refs eslint#68
@hildjj
Copy link
Contributor Author

hildjj commented Dec 3, 2024

Those three PRs should cover this issue.

@nzakas
Copy link
Member

nzakas commented Dec 3, 2024

What do we think about -0?
Safe for interop?

Good question. It's definitely not a problem in JavaScript, but I'm unsure of other languages.

@hildjj
Copy link
Contributor Author

hildjj commented Dec 3, 2024

What do we think about -0?
Safe for interop?

Good question. It's definitely not a problem in JavaScript, but I'm unsure of other languages.

Let's leave it be then. 8259 doesn't say anything about it, so it's a separate issue if we decide it needs to be added.

mdjermanovic pushed a commit that referenced this issue Dec 4, 2024
* test: Matching escaped and unescaped keys.

Refs #68

* Update momoa, uncomment test and ensure it now passes.

* fmt
hildjj added a commit to hildjj/eslint-json that referenced this issue Dec 4, 2024
Flag subnormals, numbers that are flushed to zero,
and overly-large integers.

Refs eslint#68
mdjermanovic added a commit that referenced this issue Dec 4, 2024
* feat: Add top-level-interop rule.

A new rule that checks that the top-level item in
a JSON file is either an array or an object, as
suggested by RFC8259.  Not turned on in the recommended
config because the software that had this issue
is almost entirely obsolete.

Refs #68.

* Code review responses

* Update src/index.js

Co-authored-by: Milos Djermanovic <[email protected]>

* Fix quoting

---------

Co-authored-by: Nicholas C. Zakas <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
@nzakas nzakas moved this from Ready to Implement to Implementing in Triage Dec 4, 2024
mdjermanovic pushed a commit that referenced this issue Dec 7, 2024
* test: Drive-by nit to fix test name

When the test was copied from a previous example,
the name in the comments wasn't changed.

* feat: Catch more unsafe numbers

Flag subnormals, numbers that are flushed to zero,
and overly-large integers.

Refs #68

* Rework unsafe zero.  Fix code review issues.

* Add a few more tests

* Rework error text

* Move detailed description of no-unsafe-values to readme.

* fmt

* Add values to errors

* Make links in readme

* code review issues
@hildjj
Copy link
Contributor Author

hildjj commented Dec 19, 2024

I think this is now fixed. Suggest closing?

@nzakas
Copy link
Member

nzakas commented Dec 30, 2024

Yes, thanks so much.

@nzakas nzakas closed this as completed Dec 30, 2024
@github-project-automation github-project-automation bot moved this from Implementing to Complete in Triage Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Complete
Development

No branches or pull requests

2 participants