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

Upgrade Ruma #433

Merged
merged 4 commits into from
Nov 30, 2021
Merged

Upgrade Ruma #433

merged 4 commits into from
Nov 30, 2021

Conversation

jplatte
Copy link
Collaborator

@jplatte jplatte commented Nov 26, 2021

I finished the dynamically-sized identifiers work. It was a lot. Now the upgrade turns out to also be a lot. I think some of the symbol soup could be gotten rid of again if Arc was used more consistently. Currently lots of Arc<FooId> <=> Box<FooId> conversions are being done.

To be clear, this is a draft PR because I just uploaded what I got so far, there is probably no point in reviewing yet because there's still more to do. (but I would expect that I'm over halfway done...)

@jplatte jplatte marked this pull request as ready for review November 26, 2021 18:10
@jplatte
Copy link
Collaborator Author

jplatte commented Nov 26, 2021

Hope it's okay to ignore clippy::type_complexity in all the new places it's popped up.

@jplatte
Copy link
Collaborator Author

jplatte commented Nov 26, 2021

CI fails with

error: implementation of `std::ops::FnOnce` is not general enough
   --> crates/matrix-sdk-appservice/src/webserver/warp.rs:102:14
    |
102 |             .and_then(handlers::transaction)
    |              ^^^^^^^^ implementation of `std::ops::FnOnce` is not general enough
    |
    = note: closure with signature `fn(&'0 std::boxed::Box<ruma::UserId>) -> &ruma::UserId` must implement `std::ops::FnOnce<(&std::boxed::Box<ruma::UserId>,)>`, for any lifetime `'0`...
    = note: ...but it actually implements `std::ops::FnOnce<(&std::boxed::Box<ruma::UserId>,)>`

which makes absolutely no sense because handlers::transaction is neither a closure nor does it have a UserId parameter.

I have stared at the file and looked at all the (explicit) usage of UserId in matrix-sdk-appservice but am still dumbfounded.

@poljar
Copy link
Contributor

poljar commented Nov 29, 2021

Hope it's okay to ignore clippy::type_complexity in all the new places it's popped up.

Related to that, what's the reason for removing DeviceIdBox? Wouldn't having such type aliases alleviate the need for the type complexity ignores?

@jplatte
Copy link
Collaborator Author

jplatte commented Nov 29, 2021

Well, I imagine lots of use cases will actually be better off using Arc and three type aliases (Box, Rc, Arc) per identifier type might be a bit much. Also nobody really cared about them when I asked in one of the Matrix rooms a few months back.

@poljar
Copy link
Contributor

poljar commented Nov 29, 2021

Well, I imagine lots of use cases will actually be better off using Arc and three type aliases (Box, Rc, Arc) per identifier type might be a bit much. Also nobody really cared about them when I asked in one of the Matrix rooms a few months back.

Alright, I guess we'll leave it as is.

Not sure what the warp issue is about, doesn't seem to make much sense to me either. Perhaps @johannescpk might help out?

@jplatte
Copy link
Collaborator Author

jplatte commented Nov 29, 2021

This has nothing to do with warp, commenting out parts of receive_sync_response in the base client fixes it. I'm currently narrowing it down. I suspect an issue with auto traits.

@jplatte
Copy link
Collaborator Author

jplatte commented Nov 29, 2021

I fixed it. My async_auto_traits crate helped narrow it down a little bit, though actually adding those attributes to the codebase such that they can stay in seems very hard since it also resulted in different errors that I didn't fully understand.

The underlying issue seems to be with closures only being lifetime-generic in special circumstances, but not by default. I think ultimately x in |x| &**x was inferred to have the type &'0 _ (where 0 is some concrete unnameable lifetime) instead of for<'a> &'a _.
In other circumstances (see last example here) explicitly writing : &_ seems to fix it but this wasn't the case here. I had converted everything else to .map(Deref::deref) anyways though, for better readability. This was the only case I seem to have missed, and changing it too fixed the issue.

There really needs to better diagnostics for this kind of thing. It felt like debugging C++ post-monomorphization errors 👀

@jplatte
Copy link
Collaborator Author

jplatte commented Nov 29, 2021

@jplatte
Copy link
Collaborator Author

jplatte commented Nov 29, 2021

@poljar CI fixed :)

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's quite the PR, thanks.

@poljar poljar merged commit 8494f10 into matrix-org:main Nov 30, 2021
@jplatte jplatte deleted the up-ruma branch November 30, 2021 16:42
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.

2 participants