Defer type translation failures until IR-time.#374
Merged
copybara-service[bot] merged 1 commit intomainfrom Feb 3, 2026
Merged
Defer type translation failures until IR-time.#374copybara-service[bot] merged 1 commit intomainfrom
copybara-service[bot] merged 1 commit intomainfrom
Conversation
d17b272 to
0d2bab5
Compare
This is probably necessary for fixing b/251045039: we detect overload sets by iterating over all the functions, but if a type is unknown, we don't list it as a function. Ideally we'd _always_ generate a function ir item, even if we know it will not get bindings. Deferring errors until we are in Rust also means we can generate bindings for more functions with comprehensive fallbacks, and can record more details about the error information. It is a generally good thing. Finally, because these errors are deferred to later now, I had to actually fix up the later error messages to be consistent with the errors you'd get earlier in the process. This has been a net improvement in error message quality. For example, if a method `Foo(wchar_t) volatile` doesn't get bindings, because of both the unsupported `volatile` qualifier on `this`, and the unsupported first argument, it will explicitly name `this` and `Parameter #0`, instead of `Parameter #0` and `Parameter #`, respectively. The only very bad thing that I saw in this whole process was that our bridge type handling for std::string hid a failure in how we handle templates with bad types. The old version of the code accidentally failed for wstring while it was checking for string. Reproducing this behavior with deferred "errors are actually valid values" kinds of semantics is a bit of a hack. I'm not entirely sure what the right fix is -- maybe check all the template arguments are valid for an arbitrary Record template instantiation? This also shrinks our error messages, I guess because we chain slightly fewer errors together during `ASSIGN_OR_RETURN` and so on. It doesn't matter a whole lot, as we name the function and what part of it failed (e.g. return type), as well as the root cause (e.g. wchar_t). I did have to explicitly work to retain some of the chaining, or else valuable information gets lost: an anonymous enum just gets `Unsupported type: ''`. Oof! PiperOrigin-RevId: 864992530
0d2bab5 to
d648f66
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defer type translation failures until IR-time.
This is probably necessary for fixing b/251045039: we detect overload sets by iterating over all the functions, but if a type is unknown, we don't list it as a function. Ideally we'd always generate a function ir item, even if we know it will not get bindings.
Deferring errors until we are in Rust also means we can generate bindings for more functions with comprehensive fallbacks, and can record more details about the error information. It is a generally good thing.
Finally, because these errors are deferred to later now, I had to actually fix up the later error messages to be consistent with the errors you'd get earlier in the process. This has been a net improvement in error message quality. For example, if a method
Foo(wchar_t) volatiledoesn't get bindings, because of both the unsupportedvolatilequalifier onthis, and the unsupported first argument, it will explicitly namethisandParameter #0, instead ofParameter #0andParameter #, respectively.The only very bad thing that I saw in this whole process was that our bridge type handling for std::string hid a failure in how we handle templates with bad types. The old version of the code accidentally failed for wstring while it was checking for string. Reproducing this behavior with deferred "errors are actually valid values" kinds of semantics is a bit of a hack. I'm not entirely sure what the right fix is -- maybe check all the template arguments are valid for an arbitrary Record template instantiation?
This also shrinks our error messages, I guess because we chain slightly fewer errors together during
ASSIGN_OR_RETURNand so on. It doesn't matter a whole lot, as we name the function and what part of it failed (e.g. return type), as well as the root cause (e.g. wchar_t). I did have to explicitly work to retain some of the chaining, or else valuable information gets lost: an anonymous enum just getsUnsupported type: ''. Oof!