Skip to content
This repository has been archived by the owner on Apr 5, 2024. It is now read-only.

Work around async_trait behavior #295

Merged
merged 2 commits into from
May 15, 2022
Merged

Conversation

dario23
Copy link
Contributor

@dario23 dario23 commented May 13, 2022

The #[async_trait] attribute/crate does a transformation to all async
methods, which as far as i can tell removes the &self lifetime, so our
logic to offset by 1 if has_self == true for both compared types has
an off-by-one-error here. This didn't error out earlier, since
get_region_from_params uses Vec::get, so "this index is out of
bounds" is just as None as "this generic param is not of kind
lifetime".

Also this is more a workaround than a fix. I'm not sure if we can do
something cleverer than "check if the last lifetim's name is
'async_trait".

@JohnTitor
Copy link
Member

Somehow could we have a regression test?

@dario23
Copy link
Contributor Author

dario23 commented May 14, 2022

hm, which direction of regression? i looked into adding an example testcase using #[async_trait], but async fn in general needs edition 2018, while the other tests are built without any edition flag in the rustc invocation, so they assume edition 2015, and would need changes for building with edition 2018. i think we might want that anyway, WDYT?

in the other direction i'm not so sure, i.e. how would we test that we're not creating regressions with this for other code, without knowing properly what the other code now regressing looks like; we don't know what we haven't thought of, i guess.

@dario23
Copy link
Contributor Author

dario23 commented May 14, 2022

ah, and also we'd need the async-trait crate when building old.rs and new.rs in that case.. which makes the rustc invocation quite a bit more involved (need to build async-trait crate first, including dependencies syn, quote and proc-macro2, and pass it to the other rustc invocations). i think it might be a lot easier to do a full case using casbin:2.0.0 -> casbin:2.0.1 for example.

@JohnTitor
Copy link
Member

So, you expect this PR to fix #271, right?
I mean "a test to check if the fix doesn't regress", we could add a crate-based test here, and don't have to check old/new outputs, just have to check it doesn't trigger an ICE.

@dario23
Copy link
Contributor Author

dario23 commented May 14, 2022

this should work after #291 is merged.

The `#[async_trait]` attribute/crate does a transformation to all async
methods, which as far as i can tell removes the `&self` lifetime, so our
logic to offset by 1 if `has_self == true` for both compared types has
an off-by-one-error here. This didn't error out earlier, since
`get_region_from_params` uses `Vec::get`, so "this index is out of
bounds" is just as `None` as "this generic param is not of kind
lifetime".

Also this is more a workaround than a fix. I'm not sure if we can do
something cleverer than "check if the last lifetim's name is
`'async_trait`".
@dario23
Copy link
Contributor Author

dario23 commented May 15, 2022

update: i keep getting different results for casbin running the tests locally, some are just order of errors, but there's also two errors that don't come up every run :-/

@dario23
Copy link
Contributor Author

dario23 commented May 15, 2022

alright, found one that (at least locally) doesn't show the nondeterministic behavior.

This uses `#[async_trait]`, so this test failing might indicate
something in that direction being wrong. Of course since it's a full
package being tested, it's not really accurate to just this issue, but
it's something.
@JohnTitor
Copy link
Member

Thanks! @bors r+

@bors
Copy link
Contributor

bors commented May 15, 2022

📌 Commit 75875d4 has been approved by JohnTitor

@bors
Copy link
Contributor

bors commented May 15, 2022

⌛ Testing commit 75875d4 with merge 71d2bb1...

@bors
Copy link
Contributor

bors commented May 15, 2022

☀️ Test successful - checks-actions
Approved by: JohnTitor
Pushing 71d2bb1 to master...

@bors bors merged commit 71d2bb1 into rust-lang:master May 15, 2022
@dario23 dario23 deleted the async-trait-hack branch May 17, 2022 20:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants