fix: delete_func_by_name in ModuleImports/ModuleExports
#251
Conversation
The functions `ModuleExports::delete_func_by_name` and `ModuleImports::delete_func_by_name` were broken due to the possibility of the *same* function ID being used in *multiple* exports. Since the code was getting the "first" function ID it found with a matching name (or name & module in the case of imports), deleting that export could mean deleting a *different* export ahead of time. A worked example: - export "A" w/ ID 24 exists, tied to fn 48 - export "B" w/ ID 99 exists, tied to fn 48 Things are fine when we use the export name "A" to get the relevant function ID, but when we use that ID to *look up* an export, it's unclear which one we will get -- searching by function ID would surface *either* existing export. The existing functions don't actually account for this possibility at all (they just happily return the "first" one). This commit fixes this by changing the logic of `delete_func_by_name` to use *only* the export name, and do manual checking for whether the export/import is a function. Signed-off-by: Victor Adossi <vadossi@cosmonic.com>
There was a problem hiding this comment.
Thanks for the fix here! Strictly speaking, an imported function will always have a unique ID so we should be okay with get_imported_func(fid) but this does make me think any operations on exports by function ID are likely not well-defined... is there something we should do about get_exported_func and others?
Really appreciated for iterating on these designs as necessary.
Ahhh would you like me to change this? -- I just assumed the same issue was possible there but now that you say it, it does make sense as there's no control over the mapping of imports coming in.
Yeah I'd defer here -- I'm not as strong on what the spec requires/guarantees and what the possibilities are. I think what we could do for that function is at least mark it deprecated and note why, then replace it with a function that returns one or more exports -- that seems pretty safe and won't break downstreams |
No need to. |
vados-cosmonic
deleted the
fix/module-import-or-export-delete-func-by-name
branch
The functions
ModuleExports::delete_func_by_nameandModuleImports::delete_func_by_namewere broken due to the possibility of the same function ID being used in multiple exports.Since the code was getting the "first" function ID it found with a matching name (or name & module in the case of imports), deleting that export could mean deleting a different export ahead of time.
A worked example:
Things are fine when we use the export name "A" to get the relevant function ID, but when we use that ID to look up an export, it's unclear which one we will get -- searching by function ID would surface either existing export. The existing functions don't actually account for this possibility at all (they just happily return the "first" one).
This commit fixes this by changing the logic of
delete_func_by_nameto use only the export name, and do manual checking for whether the export/import is a function.