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

wasm-encoder: Return Indices from builder functions where applicable #876

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

esoterra
Copy link
Contributor

@esoterra esoterra commented Jan 6, 2023

This refactor changes the returns of builder methods which create indexed entries (e.g. types) so that they return the index of the newly created entry. This means users don't need to otherwise calculate the value or know what it will be.

For example, the README no longer needs the line let type_index = 0; since it can be obtained when types.function(...) is called.

// Encode the type section.
let mut types = TypeSection::new();
let params = vec![ValType::I32, ValType::I32];
let results = vec![ValType::I32];
let type_index = types.function(params, results);
module.section(&types);

// Encode the function section.
let mut functions = FunctionSection::new();
functions.function(type_index);
module.section(&functions);

Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

I don't think this change makes sense for the component model sections because there isn't a 1:1 with section and index space like there is in core modules.

If we pare this down to only the module type sections, then I think this would be fine.

crates/wasm-encoder/src/component/aliases.rs Outdated Show resolved Hide resolved
@esoterra
Copy link
Contributor Author

esoterra commented Jan 6, 2023

That makes sense, I've reverted all the component model indexing logic for now and we can revisit it at a future point.
@alexcrichton mentioned that for core wasm it's also sometimes incorrect since imports exist at the beginning of the index space for some things.

From the spec:

The index space for functions, tables, memories and globals includes respective imports declared in the same module. The indices of these imports precede the indices of other definitions in the same index space

Reviewing the index spaces, only a few make sense to include:

  • typeidx
  • funcidx (includes imports)
  • tableidx (includes imports)
  • memidx (includes imports)
  • globalidx (includes imports)
  • elemidx
  • dataidx
  • localidx (not created by builder)
  • labelidx (not created by builder)

So, I'm going to further pare this down to types, elements, and data.
It's a little sad, because I think memory and table indexes being used in data and elements is one of the nicer use cases but this is all we can currently do soundly without a bigger change.

@peterhuene
Copy link
Member

@alexcrichton is, as usual, correct! We'd have to move to a pattern similar to the nested encoders in component type encoding where the module being encoded hands out new section encoders so that the index spaces are tracked across all sections being encoded in the module.

That'd be a pretty big breaking change, although it sure would be nice if component encoding tools didn't have to manually track the index spaces as component index spaces are a bit more complicated than index spaces in a module.

Stepping back for a second, I do wonder if it's worth taking the pared-down change when the index of the added item can be retrieved from the section with the len method before the call to insert a section item already; granted, that's not as nice an interface of "here's the index of what you just added", but I do wonder if it's worth changing part of the encoding API for a relatively minor ergonomic benefit.

@alexcrichton
Copy link
Member

I personally think it's ok to remove the -> &mut Self builder-style pattern since I don't think it's really used all that often with wasm-encoder anyway. In that sesnse I think returning -> u32 is fine, but while I don't think the FuncSection could return raw indices it could still have something like FuncSection::count(&self) -> u32 which returns the count of items added there. The documentation for the function could indicate that the count must be added to the number of imports to find the actual index.

For consistency though it might make the most sense to have len-style methods on most section builders rather than some returning indices and some returning &mut Self perhaps?

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.

3 participants