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

Allow non-literal constants in typescript_custom_section #3308

Closed
wants to merge 1 commit into from

Conversation

71
Copy link

@71 71 commented Feb 15, 2023

Also:

  • Bump version to 0.2.84, since the internal schema is changing.
  • Bump MSRV for UI tests to 1.57 to allow assert! in const fns.
  • Update UI test outputs.

This will, among other things, allow include_str! in #[wasm_bindgen(typescript_custom_section)] (and fix #2828). This also opens the door for non-literal constants in inline_js (thus fixing #2787) with minimal changes.

This is a clone of #3115, which was closed by GitHub when I renamed the main branch.

Also:
- Bump version to 0.2.84, since the internal schema is changing.
- Bump MSRV for UI tests to 1.57 to allow `assert!` in `const fn`s.
- Update UI test outputs.
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I can't really review the TS part (which is just a test, I know), so I will ping another maintainer when we figure things out internally.

Obviously needs a rebase.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of stuff should go into the __rt module, which you will find in lib.rs:

pub mod __rt {

Comment on lines +169 to +172
const _VALUE_LEN_VLE_LEN: usize =
wasm_bindgen::macro_support::vle_len(_VALUE_LEN);
const _VALUE_LEN_VLE: [u8; _VALUE_LEN_VLE_LEN] =
wasm_bindgen::macro_support::vle(_VALUE_LEN);
Copy link
Collaborator

@daxpedda daxpedda May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is VLE really necessary here? Just always using 4 bytes sounds good enough to me.

const _GENERATED_ARGS_LEN: usize =
wasm_bindgen::macro_support::concat_len(_GENERATED_ARGS);
const _GENERATED_LEN: usize =
#prefix_json_literal_len_literal + 4 + _GENERATED_ARGS_LEN;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just future-proofing for wasm64, don't laugh.

Suggested change
#prefix_json_literal_len_literal + 4 + _GENERATED_ARGS_LEN;
#prefix_json_literal_len_literal + std::mem::size_of::<usize>() + _GENERATED_ARGS_LEN;

@@ -337,32 +343,57 @@ fn shared_struct_field<'a>(s: &'a ast::StructField, _intern: &'a Interner) -> St
}
}

fn shared_literal_or_expression<'a>(
s: &'a ast::LiteralOrExpression,
_intern: &'a Interner,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover?

Comment on lines +361 to 362
slices: Vec<CustomSectionSlice>,
dst: Vec<u8>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we store everything only in Vec<CustomSectionSlice>, feels like I'm missing something.

let mut strings_i = 0;

while strings_i < strings.len() {
let string = &strings[strings_i];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let string = &strings[strings_i];
let string = strings[strings_i];

Comment on lines +157 to +162
mod syn {
/// Alias `syn::Expr` to `str` when decoding; an expression is evaluated as
/// a Rust constant during compilation, so when we decode the buffer we can
/// expect a string instead.
pub type Expr = str;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we pass this into the macro instead?

@daxpedda daxpedda added the needs review Needs a review by a maintainer. label May 11, 2023
@71
Copy link
Author

71 commented Jun 17, 2023

Sorry, I do plan to come back to this PR to respond to comments / rebase / apply suggested changes, but I've been pretty busy and haven't gotten around to doing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review Needs a review by a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

typescript_custom_section doesn't accept the result of include_str macro
3 participants