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

Unnecessary referencing may cause infinite recursion in compilation #149

Closed
momvart opened this issue Nov 16, 2024 · 4 comments · May be fixed by #150
Closed

Unnecessary referencing may cause infinite recursion in compilation #149

momvart opened this issue Nov 16, 2024 · 4 comments · May be fixed by #150
Labels

Comments

@momvart
Copy link

momvart commented Nov 16, 2024

These two default implementations are passing a &&Self instead of &Self to the target function in place of &A where A: Automaton. This is unnecessary as Self is implementing Automaton so we can pass self directly.

try_find_fwd(&self, input)

and
try_find_overlapping_fwd(&self, input, state)

Although this is not a problem by itself, it can possibly cause an infinite loop when converting an Automaton to a dynamic object (as theoretically in each call a new type is evaluated for the implementation of Automaton). (Unfortunately I cannot regenerate how I managed to cause the compile error, but it's possible!).

@momvart
Copy link
Author

momvart commented Nov 16, 2024

BTW, I'm wondering why such implementation (impl<'a, A: Automaton + ?Sized> Automaton for &'a A) should exist. I'll open a PR to remove it if you don't mind.

@BurntSushi
Copy link
Owner

That trait impl can't be removed. It's a breaking change.

I don't remember the precise reason it exists, other than to make &A implement Automaton. My recollection is that it was also related to avoiding monomorphization creating extra copies of some functions. But I am not confident in my methodology here.

As for the infinite loop, I think I need an MRE to determine whether the fault lies with this crate or not.

@momvart
Copy link
Author

momvart commented Nov 18, 2024

That trait impl can't be removed. It's a breaking change.

Fair.

As for the infinite loop, I think I need an MRE to determine whether the fault lies with this crate or not.

I don't think it's really reproducible out of the crate as Automaton is sealed. I was instrumenting the trait's implementation with some function that was casting self to a &dyn Automaton when I realized this unnecessary referencing exists.
You can try it with:

unsafe impl<'a, A: Automaton + ?Sized> Automaton for &'a A {
    #[inline(always)]
    fn start_state(&self, anchored: Anchored) -> Result<StateID, MatchError> {
        let _ = self as &dyn Automaton;
        // ...
    }
}

But it's basically an artificial problem!

@BurntSushi
Copy link
Owner

Right. I absolutely agree that you can get an infinite loop during compilation if the trait implemented incorrectly.

But all righty, I'm going to close this as a non-issue for now. Thanks for looking into this!

@BurntSushi BurntSushi closed this as not planned Won't fix, can't repro, duplicate, stale Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants