use a compile time default matcher map

[ririsoft]

This change makes the default map of matchers defined at compile time, making Infer::new much cheaper for high workload.

This change should be (please double check) fully compatible as far as SemVer is concerned.

This also allows for further enhancement such having a completely static Infer experience with functions at crate level. Infer::new would only be needed when custom matchers are needed. I will probably submit a PR for this after this one.

Cheers

[paolobarbolini]

Looks good to me. This would make it easy in the future to put Infer::get_from_path and Infer::add behind a feature flag and make this a no_std, no alloc crate.

[paolobarbolini]

Thinking more about it we could make Infer::new a const fn so that it could be used like this:

static INFER: Infer = Infer::new();

[ririsoft]

Why not ! I was just writing top level methods (that Infer would use internally) but the static INFER way can make it too.

With top level methods the API would be consumed this way:

// not necessary but this is just to highlight that here infer is an external crate.
extern crate infer;

fn my_func(buf: &[u8]) {
    match infer::get(buf) {
        Some(t) => do_other_stuff(buf),
        None => do_no_type_stuff(buf),
    }
}

With static INFER the API would be consumed this way:

// not necessary but this is just to highlight that here infer is an external crate.
extern crate infer;
use infer::INFER;

fn my_func(buf: &[u8]) {
    match INFER.get(buf) {
        Some(t) => do_other_stuff(buf),
        None => do_no_type_stuff(buf),
    }
}

I am totally fine with both, the later being very straightforward to implement. I could add a commit to this PR if you wish.

[paolobarbolini]

Let's see what @bojand thinks about it

[ririsoft]

I am afraid we won't be able to have this on stable rust for now without changing the API and the Matcher type:

function pointers in const fn are unstable
see issue #57563 <https://github.com/rust-lang/rust/issues/57563> for more information
add `#![feature(const_fn)]` to the crate attributes to enable

Forget about this, it works with a trick and without changing the API. I have some code ready for that if you want to play

[paolobarbolini]

I think I might have a workaround around it. I'll try it in the next few days.

[ririsoft]

Sorry I updated my comment at the same time. Indeed there is a workaround. I have an implementation for it. I will submit a PR once we have some feedbacks from @bojand.

[bojand]

Does it make more sense to chain MATCHER_MAP to custom iterator? Idea being that if clients add custom matchers, perhaps we should evaluate those first before we evaluate the standard ones. Just a thought.

[ririsoft]

I fully agree but I chose to keep the current behavior (custom matchers were pushed to the vector). Since we all agree I will push a new commit to this PR that does the change, but you will need to bump the Infer release to a major number (0.2 -> 0.3) since this change might affect current users. I will document that in the API.

[paolobarbolini]

We'll probably have to do a 0.3 release anyway if we add no_std support, but it's probably better to leave it as is for the moment just so we can do one last 0.2 release integrating these changes and any other non breaking changes that might come up before the 0.3 release

[ririsoft]

It is up to you guys, the commit is ready to be pushed to this PR or another one if you prefer.

[bojand]

You are right... there is a semver-breaking implication to that change. Yup lets do the release dance and keep the breaking change out of this PR and release this change as a patch version; and then we can introduce the behaviour change in subsequent release where we're introducing API changes anyway.

[bojand]

Hello, and thanks for the PR! I really appreciate the improvements and the collaboration 🙏.

I left a couple of minor comments / questions regarding the proposed changes, but otherwise looks good! When we address those we can merge this PR.

First a caveat, I don't do Rust development on a daily basis and context switch is sometimes difficult and I forget some of the mechanics of language or syntax, idioms, etc...

With regards to "top-level" API...

extern crate infer;

fn my_func(buf: &[u8]) {
    match infer::get(buf) {
        Some(t) => do_other_stuff(buf),
        None => do_no_type_stuff(buf),
    }
}

Just so I fully understand... what would the implementation of such API look like in the crate? All functions would be just be exported crate functions that refer to MATCHER_MAP to match. The calls are not tied to an instance of Infer struct at all? And then if a client needs custom matchers they would have to create an instance of Infer and add custom matchers, and within the implementation we would also have the same API that just wraps and calls the exported functions but chained with the iterator for custom matchers? Is that the general idea? Maybe I am missing something, especially around the context of the comment and issue linked in comment above. So I just want to make sure I understand the implementation details.

Purely from aesthetic perspective, perhaps this is a little bit "nicer" than having to initialize a new instance or refer to a static instance? But I think that's subjective or debatable.

Additionally wouldn't that have the (debatably relatively minor) advantage of not having an Infer instance allocated and created potentially at all if the client does not need custom matchers...?

The static instance change is relatively simple, whereas the "top-level-api-wrapped-in-instance" approach seems like a bit more rewriting and rewiring.

I feel either approach is beneficial and good, so I do not feel strongly about either one. Whatever makes sense holistically, and follows established practices is probably good.

[ririsoft]

Hello, and thanks for the PR! I really appreciate the improvements and the collaboration 🙏.

Thanks for Infer and your time reviewing those enhancement proposals!

Just so I fully understand... what would the implementation of such API look like in the crate? All functions would be just be exported crate functions that refer to MATCHER_MAP to match. The calls are not tied to an instance of Infer struct at all? And then if a client needs custom matchers they would have to create an instance of Infer and add custom matchers, and within the implementation we would also have the same API that just wraps and calls the exported functions but chained with the iterator for custom matchers? Is that the general idea?

This is exactly the idea.

Purely from aesthetic perspective, perhaps this is a little bit "nicer" than having to initialize a new instance or refer to a static instance? But I think that's subjective or debatable.

This is indeed debatable. I also believe that this is a little bit "nicer" to use: you do not need to use infer::INFER; and then INFER.get(...), you can just do infer::get(...). The added value of having a static infer::INFER is not clear to me, since crate level functions already bring the no_std and no_alloc features (I may be too optimistic for the no_alloc, I need to check, but this can be easily achieved anyway).

Additionally wouldn't that have the (debatably relatively minor) advantage of not having an Infer instance allocated and created potentially at all if the client does not need custom matchers...?

Exactly

The static instance change is relatively simple, whereas the "top-level-api-wrapped-in-instance" approach seems like a bit more rewriting and rewiring.

Yes, but we are talking about couple of minutes here. I would be pleased to do the change since I need it for an internal project of mine.

I feel either approach is beneficial and good, so I do not feel strongly about either one. Whatever makes sense holistically, and follows established practices is probably good.

I would favor the top level functions instead of static INFER: Infer unless it shows additional value since it provides a simpler usage experience.

[paolobarbolini]

This is indeed debatable. I also believe that this is a little bit "nicer" to use: you do not need to use infer::INFER; and then INFER.get(...), you can just do infer::get(...). The added value of having a static infer::INFER is not clear to me, since crate level functions already bring the no_std and no_alloc features (I may be too optimistic for the no_alloc, I need to check, but this can be easily achieved anyway).

I guess we could avoid exporting INFER ourselves while still giving users the possibility to do it themselves by making Infer::new a const function.
I completely agree on the fact that having to do infer::INFER.get(&buf) looks ugly, maybe we could declare INFER internally and just re-export every function from it for users who don't need custom matchers?

[ririsoft]

I guess we could avoid exporting INFER ourselves while still giving users the possibility to do it themselves by making Infer::new a const function.

I believe this is a great compromise.
Once this PR is merged I will submit a new one with a proposal for you to review