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

Multiple as_mut implementations prevent automatic type resolution #48

Closed
cd-work opened this issue Sep 1, 2023 · 3 comments · Fixed by #52
Closed

Multiple as_mut implementations prevent automatic type resolution #48

cd-work opened this issue Sep 1, 2023 · 3 comments · Fixed by #52
Labels
bug Something isn't working

Comments

@cd-work
Copy link
Contributor

cd-work commented Sep 1, 2023

Currently RulesetCreated has the following two as_mut implementations:

AsMut<Option<CompatLevel>> for RulesetCreated

impl AsMut<RulesetCreated> for RulesetCreated

I believe the CompatLevel one is new, which causes this previously working code to fail now:

landlock.as_mut().add_rule(rule)?;

This can be easily changed by changing it to (&mut landlock), but I believe the purpose of this as_mut call was to make non-builder usecase like this easier and with the latest Landlock git version that purpose isn't really working out anymore.

This can be easily fixed by changing one or both of these impls to use a non-trait method instead (could even leave the trait impl too if that's necessary). It's not a massive deal but I was just curious if this regression is something you were aware of.

@l0kod
Copy link
Member

l0kod commented Sep 11, 2023

Oh, I wasn't aware of this change. Could you please create a PR to fix it?

@l0kod
Copy link
Member

l0kod commented Sep 11, 2023

I'd like to add this fix (with tests) before releasing a new version.

@cd-work
Copy link
Contributor Author

cd-work commented Sep 12, 2023

Thinking about this, I'm honestly not sure if there's a simple and accessible solution to this.

The AsMut<Option<CompatLevel>> can't really be changed easily because it's required for numerous trait resolutions. So the only option would be to change the AsMut<RulesetCreated> for RulesetCreated and provide a method for that. This is also required for some traits though and using the existing as_mut name is probably a bad idea anyway since that's kinda reserved for the trait.

So the only option would be to add a different function that just turns RulesetCreated into &mut RulesetCreated and I don't think there's any solution to that which is easier than (&mut ruleset). Either way people aren't going to find this unless they explicitly look up some documentation that shows off usage like this, since it's behind so many layers of trait obfuscation.

So if 99% of people looking at the API are just going to use a builder or write ruleset = ruleset.handle_access(...); anyway, then it's probably better to not give a separate option to do the same thing and instead just streamline the process with this API.

The only place where this doesn't work so easily is when storing the ruleset on a struct and trying to call add_rule on self, which was my original motivation behind asking for this feature. But just using (&mut self.ruleset) is probably good enough to support this?

@l0kod l0kod closed this as completed in a0da8c0 Oct 3, 2023
@l0kod l0kod added the bug Something isn't working label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants