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

The proc-macro-error crate is end-of-life and unmaintained #854

Closed
audunhalland opened this issue Jan 29, 2024 · 5 comments · Fixed by #920
Closed

The proc-macro-error crate is end-of-life and unmaintained #854

audunhalland opened this issue Jan 29, 2024 · 5 comments · Fixed by #920

Comments

@audunhalland
Copy link

utoipa-gen has migrated to syn 2.x, but it has proc-macro-error as a dependency which still requires syn 1.0. This is the last crate in one of my Cargo.locks that requires it.

As can be seen on proc-macro-errors gitlab repo, it looks rather unmaintained, and an update can't be expected. This open gitlab Merge Request talks about various options for migration to other proc-macro diagnostics solutions.

After a quick look, it looks like utoipa-gen uses the abort!() strategy a lot for diagnostic reporting. This pattern seems to have gone out of fashion when I look at the alternatives to proc-macro-error, they seem to propose using a Result-based pattern instead, propagating up to the root macro function.

@juhaku
Copy link
Owner

juhaku commented Jan 29, 2024

Yes, I have been aware of this quite a long time (in fact ever since the syn 2). I have been pondering with the option going for the Result based error reporting for the proc macros and the syn crate also provides the primitives for that. The problem is that utoipa-gen is quite heavily invested into proc-macro-error and the update will be quite tedious to do, but should be done though.

@audunhalland
Copy link
Author

Maybe I'll try soon-ish to make some PRs. I can try to do things in smaller steps. If you want. The kanban board looks full of tasks!

@juhaku
Copy link
Owner

juhaku commented Jan 29, 2024

Maybe I'll try soon-ish to make some PRs. I can try to do things in smaller steps. If you want. The kanban board looks full of tasks!

Thanks, probably it is good to start form something related to ToSchema. 🤔 Or perhaps the OpenApi macro is the smallest one of them so that could be the easiest one of them to migrate.

It is 😓 I haven't had the energy to invest into this library for some time thus it has been outcasting for a while. I have been considering creating an organization page for this so it would be easier for people to work on a patch for the utoipa project. And of course a dedicated group of volunteers would help too 😄 .

@audunhalland
Copy link
Author

audunhalland commented Jan 31, 2024

I noticed two things while looking over the code:

  1. Relying on abort (or "giving up" on the first encountered error in any other way) will only report one error to the user, but there may be several problems reported in the same macro invocation, because utoipa macro inputs can be quite large with several unrelated problems.
  2. A lot of try-like things are done from within quote::ToToken impls. I don't think that trait is designed to work with fallible conversion to tokens.

I thought about introducing some &mut Context that can accumulate different errors and report all of them together as a fix for the first point. Error handling for different parts of the macro could then stay more local in their respective modules; at a higher level, the macro can just continue to run even if errors were previously reported.

But of course there's no obvious way to thread such a Context object through the various layers, because of ToTokens and its fixed signature. The next idea is to use some thread_local! that is initialized at the start of each macro, through a trampoline function which checks if there are errors after its inner function has finished executing. If the error list is non-empty it will output a generated compile_error!, otherwise it will output the produced TokenStream.

This way the error refactor should become much smaller and the ToTokens architecture can stay. What do you think @juhaku ?

juhaku added a commit that referenced this issue May 9, 2024
Missing migration from `ToResponse`, `IntoResponses`, `axum_extras` and
`rocket_extras`.

Resolves #854
@juhaku
Copy link
Owner

juhaku commented May 9, 2024

You are right about that the first error will only be reported at time. It would most likely make sense to report are errors simultaneously compared to the one at the time. This way user could react to multiple errors in one go. Yet some errors are only available after some other condition happens. It might not be a bad idea to use some sort of thread local or perhaps just some once cell variant to store the errors from one row and then print them out.

I have now done a minimal changes to the architecture to support fallible ToTokens, like you mentioned it is not designed to be fallible.

To elaborate further I have introduced ToTokensDiagnostics (named as so far, probably I will change that name later) that is syntax wise identical to the ToTokens but instead of void it will return Result<(), Diagnostics> that then is propagated up in the call stack to the main function.

@juhaku juhaku moved this to In Progress in utoipa kanban May 9, 2024
juhaku added a commit that referenced this issue May 10, 2024
Missing migration from `ToResponse`, `IntoResponses`, `axum_extras` and
`rocket_extras`.

Resolves #854
juhaku added a commit that referenced this issue May 12, 2024
Missing migration from `components`, `axum_extras` and
`rocket_extras`.

Resolves #854
juhaku added a commit that referenced this issue May 13, 2024
Missing migration from `axum_extras` and `rocket_extras`.

Resolves #854
juhaku added a commit that referenced this issue May 13, 2024
Implement `Result<T, E>` based proc macro error handling instead of out
of fashion `proc_macro_error` that still is in _syn_ `1.0`. This allows
us to remove the need for `proc_macro_error` crate and makes our
dependency tree leaner.

The error handling is implemented with custom `ToTokensDiagnostics` trait
that is used instead of the `ToTokens` when there is a possibility for
error. Error is passed up in the call stack via `Diagnostics` struct
that converts to compile error token stream at root of the macro call.

The result based approach is the recommended way of handling compile
errors in proc macros.

Resolves #854
juhaku added a commit that referenced this issue May 14, 2024
Implement `Result<T, E>` based proc macro error handling instead of out
of fashion `proc_macro_error` that still is in _syn_ `1.0`. This allows
us to remove the need for `proc_macro_error` crate and makes our
dependency tree leaner.

The error handling is implemented with custom `ToTokensDiagnostics` trait
that is used instead of the `ToTokens` when there is a possibility for
error. Error is passed up in the call stack via `Diagnostics` struct
that converts to compile error token stream at root of the macro call.

The result based approach is the recommended way of handling compile
errors in proc macros.

Resolves #854
juhaku added a commit that referenced this issue May 14, 2024
Implement `Result<T, E>` based proc macro error handling instead of out
of fashion `proc_macro_error` that still is in _syn_ `1.0`. This allows
us to remove the need for `proc_macro_error` crate and makes our
dependency tree leaner.

The error handling is implemented with custom `ToTokensDiagnostics` trait
that is used instead of the `ToTokens` when there is a possibility for
error. Error is passed up in the call stack via `Diagnostics` struct
that converts to compile error token stream at root of the macro call.

The result based approach is the recommended way of handling compile
errors in proc macros.

Resolves #854
juhaku added a commit that referenced this issue May 14, 2024
Implement `Result<T, E>` based proc macro error handling instead of out
of fashion `proc_macro_error` that still is in _syn_ `1.0`. This allows
us to remove the need for `proc_macro_error` crate and makes our
dependency tree leaner.

The error handling is implemented with custom `ToTokensDiagnostics` trait
that is used instead of the `ToTokens` when there is a possibility for
error. Error is passed up in the call stack via `Diagnostics` struct
that converts to compile error token stream at root of the macro call.

The result based approach is the recommended way of handling compile
errors in proc macros.

Resolves #854
juhaku added a commit that referenced this issue May 14, 2024
Implement `Result<T, E>` based proc macro error handling instead of out
of fashion `proc_macro_error` that still is in _syn_ `1.0`. This allows
us to remove the need for `proc_macro_error` crate and makes our
dependency tree leaner.

The error handling is implemented with custom `ToTokensDiagnostics` trait
that is used instead of the `ToTokens` when there is a possibility for
error. Error is passed up in the call stack via `Diagnostics` struct
that converts to compile error token stream at root of the macro call.

The result based approach is the recommended way of handling compile
errors in proc macros.

Resolves #854
juhaku added a commit that referenced this issue May 14, 2024
Implement `Result<T, E>` based proc macro error handling instead of out
of fashion `proc_macro_error` that still is in _syn_ `1.0`. This allows
us to remove the need for `proc_macro_error` crate and makes our
dependency tree leaner.

The error handling is implemented with custom `ToTokensDiagnostics` trait
that is used instead of the `ToTokens` when there is a possibility for
error. Error is passed up in the call stack via `Diagnostics` struct
that converts to compile error token stream at root of the macro call.

The result based approach is the recommended way of handling compile
errors in proc macros.

Resolves #854
@github-project-automation github-project-automation bot moved this from In Progress to Done in utoipa kanban May 14, 2024
@juhaku juhaku moved this from Done to Released in utoipa kanban Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Released
Development

Successfully merging a pull request may close this issue.

2 participants