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

Remove erroneous reference to the base Symbol when mapping Rust types #1439

Conversation

david-perez
Copy link
Contributor

.addReference(this) adds a reference to the Symbol on which
.mapRustType was called. This is correct only when f is a function
that wraps its input RustType; for example, when f wraps it in a
Box or constructs a Vec. However, the code is incorrect for an
arbitrary f; for example, when f swaps the type.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

`.addReference(this)` adds a reference to the `Symbol` on which
`.mapRustType` was called. This is correct only when `f` is a function
that _wraps_ its input `RustType`; for example, when `f` wraps it in a
`Box` or constructs a `Vec`. However, the code is incorrect for an
arbitrary `f`; for example, when `f` _swaps_ the type.
@david-perez david-perez requested a review from a team as a code owner June 3, 2022 12:44
@david-perez david-perez mentioned this pull request Jun 3, 2022
@github-actions
Copy link

github-actions bot commented Jun 3, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec -10.11% 33417.07 37176.31
Total requests -10.15% 3007910 3347793
Total errors NaN% 0 0
Total successes -10.15% 3007910 3347793
Average latency ms 8.89% 0.98 0.9
Minimum latency ms 0.00% 0.03 0.03
Maximum latency ms 21.80% 21.06 17.29
Stdev latency ms -1.69% 0.58 0.59
Transfer Mb -10.15% 312.67 348
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

@rcoh
Copy link
Collaborator

rcoh commented Jun 3, 2022

do you think this should just use symbol.toBuilder() instead of making a new symbol? We probably want to preserve symbol references—those are use for nested dependencies. We could also make this method private and only use it in the mapping use cases you described

@david-perez
Copy link
Contributor Author

do you think this should just use symbol.toBuilder() instead of making a new symbol? We probably want to preserve symbol references—those are use for nested dependencies.

That would also be incorrect for an arbitrary f right? f could map from e.g. Vec<http::header::HeaderName> to Option<String> --- the reference to http::header::HeaderName should not be preserved; if you do preserve it, and it's your only usage of a type from the http crate, you'll end up with a dependency on http that you don't need.

It's very unlikely that eliminating SymbolReferences upon usage of this function will cause us to stop depending on a crate that we do need. But if we do, the build will fail, which I think is a better outcome than silently depending on something we don't need.

@github-actions
Copy link

github-actions bot commented Jun 9, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@david-perez david-perez enabled auto-merge (squash) July 7, 2022 17:49
@github-actions
Copy link

github-actions bot commented Jul 7, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@david-perez david-perez merged commit 1338063 into main Jul 7, 2022
@david-perez david-perez deleted the davidpz/remove-erroneous-reference-to-the-base-_symbol_-when-mapping-rust-types branch July 7, 2022 18:23
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.

4 participants