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

Not all integer properties are indicated as such in the spec #90

Closed
KamasamaK opened this issue Jan 2, 2020 · 6 comments · Fixed by #357
Closed

Not all integer properties are indicated as such in the spec #90

KamasamaK opened this issue Jan 2, 2020 · 6 comments · Fixed by #357
Assignees
Labels
clarification Protocol clarification

Comments

@KamasamaK
Copy link
Contributor

Since the auto-generated spec file uses TypeScript to represent the protocol, the integer type from the schema is converted to number. This is mitigated in some places that have "The value should be less than or equal to 2147483647 (2^31 - 1)." as part of the comment, but that is not universally included for integer properties.

Continuation from #62 and #63

@jonahgraham
Copy link
Contributor

It seems reasonable for items such as threadIds, line, column to be stored as 32-bit integer, other items such as ReadMemoryResponse.unreadableBytes, ReadMemoryArguments.offset, DisassembleArguments.offset do refer to 64-bit address spaces. The ReadMemoryArguments.memoryReference is already a string, so perhaps it is reasonable to limit such offsets and unreadableBytes to 32-bit values.

Thank you for for providing clarification.

jonahgraham added a commit to eclipse-lsp4j/lsp4j that referenced this issue Jan 27, 2020
The initial implementation of lsp4j.debug used Long to map JSON's
integer values as the debug protocol spec did not provide range
information.

See also microsoft/debug-adapter-protocol#90
jonahgraham added a commit to eclipse-lsp4j/lsp4j that referenced this issue Jan 27, 2020
The initial implementation of lsp4j.debug used Long to map JSON's
integer values as the debug protocol spec did not provide range
information.

See also microsoft/debug-adapter-protocol#90
jonahgraham added a commit to eclipse-lsp4j/lsp4j that referenced this issue Jan 30, 2020
The initial implementation of lsp4j.debug used Long to map JSON's
integer values as the debug protocol spec did not provide range
information.

See also microsoft/debug-adapter-protocol#90
@jonahgraham
Copy link
Contributor

@weinand can we get a confirmation that these should be 32-bits in the spec? LSP4J wants to do a release soon and we are assuming that based on the referenced issues that this is indeed supposed to be 32-bit integers.

vladdu pushed a commit to vladdu/lsp4j that referenced this issue Jan 26, 2021
…d of Long

The initial implementation of lsp4j.debug used Long to map JSON's
integer values as the debug protocol spec did not provide range
information.

See also microsoft/debug-adapter-protocol#90
@KamasamaK
Copy link
Contributor Author

I would recommend following the convention used by the Language Server Protocol of defining a custom base type in the TypeScript spec.

This way, it will be clear that the type is integer instead of number without having to refer to the docblock.

The type definition copied from LSP:

/**
 * Defines an integer number in the range of -2^31 to 2^31 - 1.
 */
export type integer = number;

@connor4312
Copy link
Member

connor4312 commented Nov 16, 2022

Saw this referenced while looking through another issue. I propose adding the following to the "Content Part" of the overview.md:

integers defined in the protocol may be represented as 32 bit signed integers, although some properties disallow negative values. numbers in the protocol may be represented a 64 bit floating point numbers.

I believe this restriction to be appropriate based on a review of type usage. As long as there are no files more than 2^31-1 lines long...

@weinand
Copy link
Contributor

weinand commented Nov 22, 2022

@connor4312 I fully agree with your assessment.

@KamasamaK
Copy link
Contributor Author

I realize I may not have been clear in my description, but the change in #357 doesn't solve the issue I was trying to describe. This wasn't about just specifying what integer means in the protocol.

Basically, the source of truth seems to be the JSON schema. But the published specification that is auto-generated from that schema does not properly retain the integer types. Since integer is not a native type in TypeScript, I was recommending making a custom type as LSP does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Protocol clarification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants