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

Check the number of type parameters before handling the Range type #45

Merged
merged 8 commits into from
Nov 18, 2024

Conversation

todays-mitsui
Copy link

There is an issue where a user-defined Range type is incorrectly treated as std::ops::Range.
The behavior differs depending on whether the type has exactly one type parameter or not, but in either case, the user-defined Range is ignored.

Example: A panic occurs when there are no type parameters.

use tsify_next::Tsify;

#[derive(Tsify)]
pub struct Range {}

#[derive(Tsify)]
//       ^^^^^ error: proc-macro derive panicked
//             help: message: index out of bounds: the len is 0 but the index is 0
pub struct A {
    range: Range,
}

When the Range type is specified for a field in a struct, Tsify expects it to have one type parameter.
However, a user-defined Range type without type parameters causes a panic.

Example: When there is one type parameter, it is treated as std::ops::Range.

use tsify_next::Tsify;

#[derive(Tsify)]
pub struct Range<T> {
    _phantom: std::marker::PhantomData<T>,
}

#[derive(Tsify)]
pub struct A {
    range: Range<()>,
}

Generated .d.ts file:

export interface Range {}

export interface A {
    range: { start: null; end: null };
    // Expected: `range: Range;`
}

This issue arises because the system cannot correctly determine whether Range refers to a user-defined type or std::ops::Range.
Unfortunately, avoiding this issue when there is exactly one type parameter seems difficult.
On the other hand, for cases with a different number of type parameters, the distinction is straightforward. Clearly, Range or Range<T, U> should not be interpreted as std::ops::Range.

Adding a guard to TsType::from_name() to handle cases with exactly one type parameter may provide a solution to this problem.

@todays-mitsui todays-mitsui changed the title Check argments length before handling Range type Check the number of type parameters before handling the Range type Nov 16, 2024
@todays-mitsui todays-mitsui marked this pull request as ready for review November 16, 2024 15:31
@siefkenj
Copy link
Owner

Hmm. Looks like the expandtest tests are failing. Possibly because they are using a newer rust version than when they were last run...would you mind rebuilding those with MACROTEST=overwrite cargo test?

@todays-mitsui
Copy link
Author

I ran MACROTEST=overwrite cargo test and committed the changes in the tests/expand/ directory.
ee02def

I also ran ./test.sh locally, and it seems that all the tests passed.

@siefkenj
Copy link
Owner

Very weird. There are new tests failing. Do they fail on your system?

@todays-mitsui
Copy link
Author

todays-mitsui commented Nov 18, 2024

Looking at the workflow logs, it seems the test is failing during the execution of wasm-pack test --node.
On my local environment, this test passed without issues.

I tried a solution in the forked repository.
It appears that the default version of Node on ubuntu-latest is v18.20.5, which seems outdated.
By upgrading the Node version to 20, wasm-pack test --node passes successfully.

@todays-mitsui
Copy link
Author

Oops, it seems the e2e test is still failing even after upgrading the Node version.
This fix looks straightforward—perhaps a mistake caused some spaces in the snapshot to be removed?

I'll create a Pull Request to address this workflow issue.

@todays-mitsui
Copy link
Author

I have created a Pull Request to fix the workflow. Please review it.

Once the workflow completes without errors, it should be easier to review and accept this Pull Request.

@siefkenj siefkenj merged commit 39acd09 into siefkenj:main Nov 18, 2024
3 checks passed
@siefkenj
Copy link
Owner

Thanks for this! And thank you for adding tests right from the start :-D

@todays-mitsui todays-mitsui deleted the check_args_len_for_range_type branch November 18, 2024 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants