Rust code generation independent of FaustDsp trait #1062

crop2000 · 2024-10-09T22:54:10Z

faust code generation should not specify traits.
this pr introduces this change.
It is tested against the impulse-tests using the architecture file in its subfolder.
How much architecture files need to change depends on how traits are used.

The jack cpal and minimal architecture files could be compiled after minimal changes,
but need more cleanup work.

sletz · 2024-10-10T10:51:53Z

Thanks. Testing here with faust2portaudiorust tool, but I gets runtime error like:

Faust Rust code running with Portaudio: sample-rate = 44100 buffer-size = 64
get_num_inputs: 1
get_num_outputs: 2
thread 'main' panicked at src/main.rs:227:11:
called `Result::unwrap()` on an `Err` value: InvalidChannelCount
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

so I guess the current port audio architecture, which use a fixed const CHANNELS: i32 = 2; will have to be adapted.

Can you have a look ?

sletz · 2024-10-10T11:35:32Z

Merged thanks.

crop2000 · 2024-10-10T14:57:43Z

ah now i understand the context of the other comment on the other pr better ... yes i will have a look.

bluenote10 · 2024-10-11T07:30:15Z

Could we wait a little bit with making this change? I have a substantial 'productive' usage of Rust + Faust, and I would like to test how this change would impact it. Unfortunately I'm travelling right now, and will only have access to a PC in a few days again.

Specifically I want to verify if using ambiguous method vs impl names doesn't cause issues (hopefully it doesn't require using fully qualified names). Also, I'm seeing that some of the trait methods have been removed, and I'm wondering if it is easy to work around the missing functionality.

sletz · 2024-10-11T08:12:27Z

The merged commit is here: 28a8b1a and is not involving any change in the architectures files. So hopefully it should not break anything. Please yes give feedback.

In a more general term, @crop2000 we definitively have to be quite careful. As I said, I don't have expertise and time enough to be the final judge. @bluenote10, @obsoleszenz at least have to give their feedback each time.

obsoleszenz · 2024-10-11T10:22:40Z

Could we wait a little bit with making this change? I have a substantial 'productive' usage of Rust + Faust, and I would like to test how this change would impact it. Unfortunately I'm travelling right now, and will only have access to a PC in a few days again.

Specifically I want to verify if using ambiguous method vs impl names doesn't cause issues (hopefully it doesn't require using fully qualified names). Also, I'm seeing that some of the trait methods have been removed, and I'm wondering if it is easy to work around the missing functionality.

Agree. Let's move this one carefully. Maybe first introduce it behind a feature flag. But for sure am with @crop2000 that the rust code gen could be improved a good bit.

crop2000 · 2024-10-11T10:42:54Z

@sletz yes commit 28a8... does not change the interface at all. It extents it providing a new function.

@bluenote10 when you are able to share your architecture file it would be interesting. I don't aim to rush this PR, but i like to keep it minimal so i don't want to improve the architecture files in this PR. Just want to keep them working in the context they worked before. (Outdated jack etc...)

crop2000 · 2024-10-11T10:49:57Z

@obsoleszenz a feature flag would increase the maintainance burden. Because it would double the code.

I rather try to find a solution that works with a drop in extension, that reproduces the old behavior.

bluenote10 · 2024-10-13T17:15:18Z

The merged commit is here: 28a8b1a and is not involving any change in the architectures files.

I can confirm that 28a8b1a works well for me, and I like the idea of having that compute_array function.

when you are able to share your architecture file it would be interesting.

My architecture file is rather boring:

use crate::types::{FaustDsp, Meta, UI, ParamIndex};

type F32 = f32;

// Generated intrinsics:
<<includeIntrinsic>>

// Generated class:
<<includeclass>>

What is crucial in my use case it to have a common interface for all generated DSPs (I have many), i.e., I naturally need a trait, and all generated DSPs are supposed to re-use the same base types.

I have experimented a little bit with this feature branch, and I was able to make my usages work with it as well (by also forwarding the missing methods). So in principle I would be fine with the change.

However, I don't really see yet why we want to go in that direction. So far I see a few minor downsides:

Now that DSPs are not implemented against an interface out-of-the-box, new developers may wonder how to arrive at a "common denominator" for the generated output (wasn't the original code generation non trait based, and we introduced traits to make the set of methods more obvious?). In other words, it is not obvious that forwarding the various self.xxx calls in the architecture file into a trait is generally valid, because it is not clear whether one can rely on a particular self.xxx being generated.
For use cases that want a trait, the forwarding of all method calls into a trait makes the generated code larger: Every dsp will contain the regular impl block plus the trait forwarding boilerplate. Not a big problem, but avoiding the boilerplate part would be nicer.
When using the same method in the traits as the underlying inherent function names, one may start wondering whether the ambiguity can cause issues (worst case would be an infinite recursion, if e.g. self.compute would call the trait method instead of the inherent function). Most likely this should not be an issue, because fortunately Rust behaves as desired in this case, but it creates at least unnecessary questions. Also note that there seems to be a clippy lint (Lint for ambiguous method defined in trait impl for struct & on struct itself rust-lang/rust-clippy#11499) that warns about such ambiguities. Using different method names in the trait to avoid the ambiguity is also not ideal due to reduced readability.

If I understand it correctly, this change was motivated by supporting -ec which (probably) requires different signatures e.g. of the compute method. What would be the implications of keeping the FaustDsp trait, but treating its method signatures as "variable"? I.e., if invoked with -ec switch the codegen would expect the FaustDsp trait (injected via the architecture file) to look slightly different. This would probably imply that one has to use different architecture files depending on the command line switches. Is that what you are trying to avoid?

crop2000 · 2024-10-13T20:18:57Z

Hi @bluenote10,
Thanks for your reply.

My motivation for this and following changes is to keep the code generated by faust small and at the same time make the interface more flexible.

I consider the FaustDsp trait flawed. It seems to me that it is designed like a object. It has for example the anti pattern to specify a new function for the trait.

The functions I removed from the trait cannot be called from a trait object, because they don't have a self argument. The following will not work:

let i = FaustDsp::new();
i.class_init(44000)
i.build_user_interface_static(ui_interface);

Did you try to generate your code with those functions removed from your trait specification?

did you see this discussion: #1059

crop2000 · 2024-10-14T12:11:54Z

@bluenote10 I have an idea.

we move this block

faust/tests/impulse-tests/archs/rust/architecture.rs

Line 246 in 2026d7a

impl FaustDsp for mydsp {

into the code generated by faust (with the 3 functions i criticized above).
and add a flag like --no-faustdsp-trait
to make it optional.

This allows us to be fully backward compatible.
And have a separation of concerns (code generated by faust, traits, backward compatibility).
And we can change the underlying implementations without affecting current architecture files.

In the long run I think we can come up with traits that are better specified ... the FaustDsp implementation would in that case redirect to those implementations.

This doesn't solve the issue with the clippy lint you mentioned but this lint would appear in code generated by rust and not in the architecture file so i think this would be less of a problem.

bluenote10 · 2024-10-14T12:25:26Z

My motivation for this and following changes is to keep the code generated by faust small

For use cases that need a trait the generated code is significantly larger due to having two impl blocks now. My git stats on my project after incorporating this change says ~500 lines of code more.

I consider the FaustDsp trait flawed. It seems to me that it is designed like a object.

That is rather vague. The trait is not object safe, it is supposed to be used statically / generically.

It has for example the anti pattern to specify a new function for the trait.

To clarify: Having static factory methods in a trait is per se obviously not an anti-pattern -- it's basically just what the Default trait does. You are probably referring to not using Default to begin with? The idea here was to eventually change the signature to new(sample_rate: u32) -> Self to avoid always to set the sample rate after the construction (which is madatory before usage anyway). If the static new method has a signature different from Default (because DSPs cannot really be default constructed without specifying the sample rate) then the anti-pattern is gone.

The functions I removed from the trait cannot be called from a trait object, because they don't have a self argument.

Well, of course, they are static factory methods, so you have to use them like e.g. MyDspType::build_user_interface_static(ui_interface) or via generics.

Did you try to generate your code with those functions removed from your trait specification?

Yes, but then everything breaks for me. My usages are all generic based on the trait. Think of usages like:

pub fn create<D: FaustDsp<T = AudioFloat>>(sample_rate: i32) -> D {
    let mut instance = D::new();
    instance.init(sample_rate);
    instance
}

pub fn extract_param_defs<D: FaustDsp<T = AudioFloat>>() -> Vec<DspParamDef>
where
    T: Clone,
{
    let mut ui_builder = DeclarativeUIBuilder::new();
    D::build_user_interface_static(&mut ui_builder);
    ui_builder.get_param_defs()
}

For the record, I have pulled out the history where we switched from non-trait to trait: #448 At that time, this was generally preferred by e.g. @bkfox as well (#409 (comment)), because their use case also required having a common interface for all generated DSPs.

I think it basically comes down to:

My motivation for this and following changes is to [...] make the interface more flexible.

The question is how flexible does it have to be? The flip side of flexibility is instability and accidental breaking changes.

For usages that need a common denominator for multiple generated DSPs arbitrary flexibility can be detrimental: Each method generated inside the impl block would raise the question: Is this method guaranteed to exist in all DSPs? Is this method signature always the same (e.g. compute_arrays already isn't). Implementing the code gen against an interface/trait brings clarity to these questions, because it documents these are the methods you can expect to get from the code generation. If the code generation would fail to generate a method, generates additional methods, or generates different signatures, the Rust compilation would fail with a clear error message. Without a trait it would be much easier that a contributor to Faust opens PR that removes/modifies certain function signatures without noticing, which are breaking changes to Rust users. So far it was relatively easy for me to keep track of such breaking changes, because I could simply look at modifications to the trait in the architecture file, which served as a documentation of what to expect from the code generation output.

did you see this discussion: #1059

Yes, I wasn't really sure where to comment. Feel free to move the discussion back to that issue.

bluenote10 · 2024-10-14T12:35:21Z

we move this block [...] into the code generated by faust (with the 3 functions i criticized above).

That is how it currently works, right?

faust/compiler/generator/rust/rust_code_container.cpp

Line 289 in eaead83

*fOut << "impl FaustDsp for " << fKlassName << " {";

and add a flag like --no-faustdsp-trait to make it optional.

This could work, but doesn't it make the generation rather complex because it needs to support placing the method in either impl blocks?

I would love to understand how much flexibility we really need to make up my mind if giving up on traits is really better.

architecture/rust/jack-float.rs

obsoleszenz · 2024-10-14T13:38:19Z

I find it difficult to really see what this pr changes, can we have some before/after code generation example?

crop2000 · 2024-10-14T14:56:04Z

@obsoleszenz

https://gist.github.com/crop2000/9f6a0c628df5c920c6f83e3ea5df5540 as generated in the impulse-tests using https://github.com/crop2000/faust/blob/rust_no_traits/tests/impulse-tests/archs/rust/architecture.rs

please ignore the changes to the other architecture files for now.

crop2000 · 2024-10-14T14:59:35Z

@bluenote10
when you want to use MyDspType::build_user_interface_static(ui_interface) or new() why not implement it on the struct?

I will make a commit with the proposed changed so that we don't have misunderstandings about what i propose.

crop2000 · 2024-10-14T15:39:16Z

one point about what i think is important about traits they should only define what is needed for one task. In our case the information for UI and the compute function are not directly related and should have separate traits.

It would be very bad to let the generated code change the FaustDsp trait itself
so that all code that expects FaustDsp following a certain definition would fail.

For example by including compute_arrays() to FaustDsp
or to add control() when -ec flag is used to generate the code)

the latest proposal to keep the FaustDsp as an optional feature in the generated code helps to ensures backward compatibility as long as the trait definition is not changed.

bluenote10 · 2024-10-14T16:05:22Z

I find it difficult to really see what this pr changes, can we have some before/after code generation example?

The main change is to switch the code generation from producing a impl FaustDsp for SomeDsp { ... } block to just a impl SomeDsp { ... } block, i.e., whether to implement against an interface or not.

when you want to use MyDspType::build_user_interface_static(ui_interface) or new() why not implement it on the struct?

In case one knows the specific type e.g. MyDspType then yes, one can use that method on that specific type via MyDspType::build_user_interface_static(ui_interface). But in generic usages this is not possible: One only has some generic type D which satisfies the FaustDsp interface, so it is essential that the method exists on the trait to allow for D::build_user_interface_static(ui_interface). Its basically the same reason why e.g. the Default trait exists -- individual implementations of new() -> Self cannot be used generically.

one point about what i think is important about traits they should only define what is needed for one task. In our case the information for UI and the compute function are not directly related and should have separate traits.

That is true, but a few things to consider:

One can also overdo it in terms of splitting: E.g. one trait per function would probably not give the best user experience either.
I guess users can always split traits up themselves by defining their own split traits e.g. for compute vs build_user_interface and provide blanket impls.
Faust doesn't have a very strict separation of concerns between the DSP code and UI in general, so there is only so much you can do to disentangle the two.
I had considered splitting it up further originally, but in the end decided that the current 3 traits (FaustDsp, UI, Meta) should give the best trade-off with staying close to Faust internals and (somewhat) idiomatic Rust. Perhaps there is some more background on this in the original discussion at Rust: UI<T>::declare() and borrow #409 that fixed the originally broken UI system.

But that is essentially why I was asking about how flexible the interface should be, i.e., which parts may change. Once we have identified the variability of the traits, we could make a more informed decision how to split them up.

crop2000 · 2024-10-14T16:10:22Z

in commit e21f3f0 i moved the FaustDsp trait impl block into a function in code generation for backwards compatibility.

I see that this makes sense as a reference implementation to ensure backwards compatibility.
I like this idea because it would free us to change the underlying implementation more freely.

for example if compiled with -ec flag the implemetation for compute in FaustDsp would either panic or call self.control() before self.compute() ... just as an example that it creates space for improvements and behavior changes.

bluenote10

This change looks generally good now after the last adaptation. Forwarding from the trait methods to the inherent methods is probably a viable strategy and there is no breaking change.

compiler/generator/rust/rust_code_container.cpp

bluenote10 · 2024-10-15T06:24:07Z

compiler/generator/rust/rust_instructions.hh

@@ -287,7 +287,7 @@ class RustInstVisitor : public TextInstVisitor {
            // impls, we need a mechanism to forward the information whether to use "pub"
            // or not. In the worst case, we have to prefix the name string like "pub fname",
            // and handle the prefix here.
-            *fOut << "fn " << inst->fName;
+            *fOut << "pub fn " << inst->fName;


What is this change about? (I couldn't really figure out what it affects during the code gen.)

Is the comment actually still valid after this change, because now function are no longer only attached to a trait as the first sentence says?

i think it influenced the generation of get_num_inputs() before i changed it in the other pr.
i would remove the pub again.
Since functions are not attached it wouldn't matter if they are prefixed with "pub".
But because this is for internal functions we don't need pub.
I don't think the comment makes sense so I would remove it completely.

compiler/global.cpp

documentation/man/README.md

crop2000 · 2024-10-17T20:33:15Z

I am thinking to remove the option to not generate the trait from this PR.
The reason is that maybe a slightly different option (Generating less code) would be desirable.
But having to many options is not desirable.
Also changing the interface twice is not desirable.
So I removed those commits but might use them in another PR.

crop2000 · 2024-10-19T16:08:22Z

@bluenote10 do you agree with this pr?
Before FaustDsp was the implementation of the generated Faust code.
Now FaustDsp is a interface to the generated Faust code that allows us to change the underlying implementation, while keeping backwards compatibility.

bluenote10 · 2024-10-19T16:27:08Z

@bluenote10 do you agree with this pr?

Yes, as far as I can see this strategy should be fine.

It probably makes sense to merge #1066 first, because this PR seems to (partially?) contain the same changes, right?

crop2000 · 2024-10-19T17:28:36Z

this pr contains the commit of #1066 because it builds on it. I opened a separate pr to keep the scope of each change clear.

@bluenote10

treat FaustDsp as a interface faust code is generated as implementation on the dsp struct provide FaustFloat type alias the type alias can be used also in the architecture file to make them compatible to both f32 and f64 use consts in trait impl and remove obsolete io functions use usize for count in impl use usize for count in compute call thanks to @bluenote10 for some fixes

crop2000 · 2024-10-19T21:21:04Z

@sletz this PR is now also ready to be merged.
I removed all changes to architecture files because for the impulse-tests this proves that this pr is backward compatible.

sletz · 2024-10-19T21:42:41Z

Merged thanks. Be sure to check faust2portaudiorust and faust2jackrust tools. I had to update architecture files here: 0dbcf51

crop2000 · 2024-10-19T21:50:30Z

I will review the architecture files in the next days

sletz closed this Oct 10, 2024

sletz reopened this Oct 10, 2024

obsoleszenz reviewed Oct 14, 2024

View reviewed changes

architecture/rust/jack-float.rs Outdated Show resolved Hide resolved

bluenote10 approved these changes Oct 15, 2024

View reviewed changes

crop2000 force-pushed the rust_no_traits branch 3 times, most recently from fc2c91b to 305364b Compare October 15, 2024 18:12

crop2000 changed the title ~~Rust code generation without traits~~ Rust code generation independent of FaustDsp trait Oct 16, 2024

crop2000 force-pushed the rust_no_traits branch from 0f17e99 to fd78708 Compare October 16, 2024 18:22

bluenote10 reviewed Oct 17, 2024

View reviewed changes

compiler/global.cpp Outdated Show resolved Hide resolved

bluenote10 reviewed Oct 17, 2024

View reviewed changes

documentation/man/README.md Outdated Show resolved Hide resolved

crop2000 force-pushed the rust_no_traits branch 2 times, most recently from 807c116 to 3187d5f Compare October 17, 2024 22:32

crop2000 force-pushed the rust_no_traits branch from 3187d5f to b1c7993 Compare October 19, 2024 19:38

crop2000 force-pushed the rust_no_traits branch from b1c7993 to 34f8db6 Compare October 19, 2024 21:17

sletz merged commit 34f8db6 into grame-cncm:master-dev Oct 19, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust code generation independent of FaustDsp trait #1062

Rust code generation independent of FaustDsp trait #1062

crop2000 commented Oct 9, 2024

sletz commented Oct 10, 2024 •

edited

Loading

sletz commented Oct 10, 2024

crop2000 commented Oct 10, 2024

bluenote10 commented Oct 11, 2024

sletz commented Oct 11, 2024

obsoleszenz commented Oct 11, 2024

crop2000 commented Oct 11, 2024

crop2000 commented Oct 11, 2024 •

edited

Loading

bluenote10 commented Oct 13, 2024

crop2000 commented Oct 13, 2024 •

edited

Loading

crop2000 commented Oct 14, 2024

bluenote10 commented Oct 14, 2024

bluenote10 commented Oct 14, 2024

obsoleszenz commented Oct 14, 2024

crop2000 commented Oct 14, 2024

crop2000 commented Oct 14, 2024 •

edited

Loading

crop2000 commented Oct 14, 2024

bluenote10 commented Oct 14, 2024

crop2000 commented Oct 14, 2024

bluenote10 left a comment

bluenote10 Oct 15, 2024

crop2000 Oct 15, 2024

crop2000 commented Oct 17, 2024 •

edited

Loading

crop2000 commented Oct 19, 2024

bluenote10 commented Oct 19, 2024

crop2000 commented Oct 19, 2024

crop2000 commented Oct 19, 2024

sletz commented Oct 19, 2024

crop2000 commented Oct 19, 2024

Rust code generation independent of FaustDsp trait #1062

Rust code generation independent of FaustDsp trait #1062

Conversation

crop2000 commented Oct 9, 2024

sletz commented Oct 10, 2024 • edited Loading

sletz commented Oct 10, 2024

crop2000 commented Oct 10, 2024

bluenote10 commented Oct 11, 2024

sletz commented Oct 11, 2024

obsoleszenz commented Oct 11, 2024

crop2000 commented Oct 11, 2024

crop2000 commented Oct 11, 2024 • edited Loading

bluenote10 commented Oct 13, 2024

crop2000 commented Oct 13, 2024 • edited Loading

crop2000 commented Oct 14, 2024

bluenote10 commented Oct 14, 2024

bluenote10 commented Oct 14, 2024

obsoleszenz commented Oct 14, 2024

crop2000 commented Oct 14, 2024

crop2000 commented Oct 14, 2024 • edited Loading

crop2000 commented Oct 14, 2024

bluenote10 commented Oct 14, 2024

crop2000 commented Oct 14, 2024

bluenote10 left a comment

Choose a reason for hiding this comment

bluenote10 Oct 15, 2024

Choose a reason for hiding this comment

crop2000 Oct 15, 2024

Choose a reason for hiding this comment

crop2000 commented Oct 17, 2024 • edited Loading

crop2000 commented Oct 19, 2024

bluenote10 commented Oct 19, 2024

crop2000 commented Oct 19, 2024

crop2000 commented Oct 19, 2024

sletz commented Oct 19, 2024

crop2000 commented Oct 19, 2024

sletz commented Oct 10, 2024 •

edited

Loading

crop2000 commented Oct 11, 2024 •

edited

Loading

crop2000 commented Oct 13, 2024 •

edited

Loading

crop2000 commented Oct 14, 2024 •

edited

Loading

crop2000 commented Oct 17, 2024 •

edited

Loading