Remove event/emit and introduce ctx.emit that works with the Emittable trait #772

cburgdorf · 2022-07-27T15:21:15Z

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

Removed event type and emit statement
Added support for #indexed attribute on struct fields
Introduced Emittable trait which is automatically implemented for all structs
- the trait is not implementable manually (it therefore depends on a private type)
- the compiler still implements the logic behind the emit trait function internally (just like before)
updated all tests and code examples

cburgdorf · 2022-09-12T11:18:33Z

I have no idea where this "CircleCI Pipeline" job comes from. We don't use CircleCI 🤷
UPDATE: Turned out to be a sketchy trick by CircleCI to win back users. I logged into CircleCI and configured it to stop building for this project.

cburgdorf · 2022-09-13T08:30:01Z

This is now rebased on top of latest master

Y-Nak

I read through the code and think we should discuss the whole design before getting into the details.

Stopgap solutions should not affect a large part of a codebase, but this implementation seems to do so.
I think it would be enough to add a single small module that provides

semantic checks for the event features like the number of indexed fields
automatic implementation of the Emittable trait to types

What's in my mind is this module should essentially be a temporary alternative to the actual event procedural macro implementation until we provide a proc-macro-like feature, and the module will simply be removed when the day comes instead of re-modifying the various parts of the analyzer.

cburgdorf · 2022-09-14T07:26:16Z

Thanks for the review @Y-Nak!

I'm happy to discuss any alternative designs!

Stopgap solutions should not affect a large part of a codebase, but this implementation seems to do so

I think most of the parts that this PR touches are because of the places that remove the current event and all the supporting code.

semantic checks for the event features like the number of indexed fields

automatic implementation of the Emittable trait to types

I'm not arguing against implementing such a module but these two pieces seem to be very minor tweaks to the existing code so I'm not sure how problematic it realistically could be 🙃

e.g.

fe/crates/analyzer/src/namespace/items.rs

Lines 1882 to 1884 in f934794

    
           if self.is_std_trait(db, EMITTABLE_TRAIT_NAME) && ty.is_struct(db) { 
        
               return true; 
        
           }

Anyway, as I said, happy to discuss what you have in mind. I just wanted to highlight that I think most code changes in the PR are because of the removal of the current event system and very little code was added to support the temporary #indexed attribute and even less to support the Emittable trait.

Y-Nak

I enumerated the #event/#indexed specific code in the change. It seems still scattered for me. Also, almost all changes happen in the public functions, which means they potentially affect the broader part of the codebase.

Y-Nak · 2022-09-14T13:52:48Z

crates/analyzer/src/constants.rs

+pub const EMITTABLE_TRAIT_NAME: &str = "Emittable";
+pub const EMIT_FN_NAME: &str = "emit";
+pub const INDEXED: &str = "indexed";


These are #event/#indexed specific and can be used from any code in the fe compiler because it's public.

I could move these to a place closer to their usage. My reasoning for putting them here is that they are simple strings and I dislike using magic strings inline because it is easy to mistype a string without the compiler noticing (e.g. "inexed" but you can not mistype INDEXED without the compiler noticing it.

Why should INDEXED or EMITTABLE be used from several modules? I think exposing constants in an arbitrary way is not good especially if you know these constants are used as a stopgap. How long do you think these constants will remain?

Y-Nak · 2022-09-14T13:57:08Z

crates/analyzer/src/db/queries/structs.rs

+        if field.is_indexed(db) {
+            indexed_count += 1;
+        }
+
+        // Multiple attributes are currently still rejected by the parser so we only need to check the name here
+        if !field.attributes(db).is_empty() && !field.is_indexed(db) {
+            let span = field.data(db).ast.kind.attributes.first().unwrap().span;
+            scope.error(
+                "Invalid attribute",
+                span,
+                "illegal name. Only `indexed` supported.",
+            );
+        }


ditto.
Also, the #event/#indexed specific check would be bloated for additional semantic checks, e.g., we have to check whether a field is encodable because some types are not encodable like enum.

Y-Nak · 2022-09-14T13:57:27Z

crates/analyzer/src/db/queries/structs.rs

+    if indexed_count > MAX_INDEXED_EVENT_FIELDS {
+        let excess_count = indexed_count - MAX_INDEXED_EVENT_FIELDS;
+
+        let mut labels = fields
+            .iter()
+            .filter_map(|(_, field)| {
+                field
+                    .is_indexed(db)
+                    .then(|| Label::primary(field.span(db), String::new()))
+            })
+            .collect::<Vec<Label>>();
+        labels.last_mut().unwrap().message = format!("{} indexed fields", indexed_count);
+
+        scope.fancy_error(
+            &format!(
+                "more than three indexed fields in `event {}`",
+                struct_.name(db)
+            ),
+            labels,
+            vec![format!(
+                "Note: Remove the `indexed` attribute from at least {} {}.",
+                excess_count,
+                pluralize_conditionally("field", excess_count)
+            )],
+        );
+    }


Y-Nak · 2022-09-14T13:58:09Z

crates/analyzer/src/namespace/items.rs

+    pub fn is_indexed(&self, db: &dyn AnalyzerDb) -> bool {
+        self.attributes(db).contains(&INDEXED.into())
+    }


This query is #event/#indexed specific.

Y-Nak · 2022-09-14T14:02:45Z

crates/analyzer/src/namespace/items.rs

+        if self.is_std_trait(db, EMITTABLE_TRAIT_NAME) && ty.is_struct(db) {
+            return true;
+        }


This breaks the invariant of a group of methods related to trait implementation because this function returns true even if the actual implementation doesn't exist. So all other codes which depend on the method would need additional checks to ensure the actual implementation exists.

even if the actual implementation doesn't exist

Can you elaborate on that? The method is implemented by the compiler (compiler magic basically) for any struct (well, maybe we need to restrict it a bit more because as you hinted structs with enums should maybe not implement Emittable). For any Emittable the compiler ensures that emit can be called.

If the method is properly implemented by the compiler, then why are these awkward lines necessary?
Do these types have ImplId for the Emittable trait? I'd like to expect that ImplId should exist when the is_implemented_for returns true.

There is no ImplId because there is no impl block for it. We can not yet implement the actual event logic in Fe code (in that case, there would be no need for compiler magic at all). The PR doesn't change how events are handled internally, it simply changes how they are triggered.

crates/codegen/src/db/queries/types.rs

Y-Nak · 2022-09-14T14:12:28Z

crates/mir/src/lower/function.rs

+            AnalyzerCallType::TraitValueMethod {
+                trait_id, method, ..
+            } if trait_id.is_std_trait(self.db.upcast(), EMITTABLE_TRAIT_NAME)
+                && method.name(self.db.upcast()) == EMIT_FN_NAME =>
+            {
+                let event = self.lower_method_receiver(func);
+                self.builder.emit(event, source)
+            }


This is #event/#indexed specific.

It is, but so is the status quo with the Event type and the lower_event_type function. After all, this PR doesn't aim to change the internal handling of events it only changes the trigger from being emit SomeEventType to being trait based via Emittable.emit(..).

this PR doesn't aim to change the internal handling of events

That's the point. Again, I'd like to ask how long this stopgap solution will remain. And I think the core of the codebase is affected too much just to remove the superficial syntax.

Again, I'd like to ask how long this stopgap solution will remain

I don't know but I would assume it is still a while out until we can implement the actual event logic in Fe code natively.

And I think the core of the codebase is affected too much

But wouldn't you agree that the PR actually lowered the complexity of the code base in general? I mean, look at all the event handling logic that was deleted. More code was deleted than added in this PR. It's actually very little code that was added to enable the struct + trait based event handling.

I don't think complexity could/should be evaluated by the lines of code added/deleted. And how extent the codebase is affected has almost nothing to do with the LoC added/deleted.

I think the way of abstraction introduced here makes the codebase more complicated.

There is no ImplId because there is no impl block for it.

This inconsistency didn't exist before the PR, for example. As I mentioned, this would break the invariant which the trait-related methods should provide, and if the PR is merged as is, it will force us to care about the Emittable trait wherever we handle traits. You exposed the name of the Emittable trait as a public constant, and it proves that we should treat the trait specially even now, and in the future, we would start writing other stopgaps to adapt to the unexpected behavior of the core functions. So stopgaps will be chained and expanded.

Also, this PR "hides" the stopgap behind the pseudo abstraction. This makes it difficult to remove these stopgaps when we finally reach the "right" solution. I mean Rust compiler won't complain about the stopgaps in the form of compile errors. So, as a result, we'll have to find and remove them manually. I am not confident in remembering the exact functions and locations where stopgaps are embedded. And as I said, more and more stopgaps will be chained/expanded as time passes.

So, to be honest, I have to say this PR increases the complexity of the code, and that's why I strongly disagree with merging this PR as is.

No worries, we can try to work out a solution when we co-work in Berlin next week. And if we can not find a satisfying solution than I can live with the current system based on event and emit until we have all the ingredients to fully implement something like the Emittable trait in Fe itself.

That said, I do disagree on the question of complexity. I think having the event type and the emit keyword with all of its extra checks is more complex than having a trait in the std lib that is automagically implemented by the compiler as a temporary solution. Especially since it is also ensured that no one else can try to implement it manually for the time being.

I don't disagree with adding the Emittable trait and implementing it to some types, though I don't think it should be implemented to all types. What I disagree with is the way of realizing it. Again, I still think we shouldn't arbitrarily tweak the core functions on which many other parts depend.

But yeah, we will be able to discuss this face-to-face the next week, so it'd be fine to stop the discussion on GH.

crates/mir/src/ir/types.rs

Y-Nak

I added comments, which are not directly related to the current discussion but feel worth noting.

Y-Nak · 2022-09-14T14:33:04Z

crates/mir/src/db/queries/structs.rs

@@ -11,7 +11,7 @@ pub fn mir_lower_struct_all_functions(
    struct_
        .all_functions(db.upcast())
        .iter()
-        .map(|func| db.mir_lowered_func_signature(*func))
+        .map(|func| db.mir_lowered_pseudo_monomorphized_func_signature(*func))


It seems weird to use the function because it is annotated as NOTE: THIS SHOULD ONLY BE USED IN TEST CODE.

Ah, yes...I guess I forgot that this is also used by the graphviz stuff. I guess what I meant to say is that it should not be used in the actual compilation pipeline. The reason it is needed at all is because the std lib test start failing as soon as the std lib included generic code because the std lib tests iterate over these functions without them being actually called which means we can not monomorphize the generic parameters. So in order to be able to iterate over the functions I created a function that would populate them in a way that maps each generic param to (). There's probably a better way that I'm not seeing :)

Y-Nak · 2022-09-14T14:37:46Z

crates/library/std/src/context.fe

+pub trait Emittable {
+  fn emit(self, _ val: OutOfReachMarker);
+}


Suggested change

pub trait Emittable {

fn emit(self, _ val: OutOfReachMarker);

}

pub trait Emittable {

fn emit(self, _ ctx: Context);

}

I think exposing private types from a public function is not a good idea especially when it's a trait function, how about receiving Context instead of the OutOfReachMarker type?

how about receiving Context instead of the OutOfReachMarker type

I thought about that as well but that would mean that people can bypass the ctx.emit(..) API and instead call MyEvent().emit(ctx). That may be fine (since they are still depending on the Context but I kinda like it more if we force them to go through ctx.emit(..). But I don't have very strong feelings on this.

cburgdorf force-pushed the christoph/feat/loggable branch from f91cf57 to c258991 Compare August 30, 2022 13:33

cburgdorf force-pushed the christoph/feat/loggable branch 3 times, most recently from f1d07b6 to a29de31 Compare September 8, 2022 13:36

cburgdorf changed the title ~~WIP~~ WIP: Remove event/emit and introduce ctx.emit that works with the Emittable trait Sep 8, 2022

cburgdorf force-pushed the christoph/feat/loggable branch 5 times, most recently from a8d046a to 53be4fe Compare September 12, 2022 11:07

cburgdorf marked this pull request as ready for review September 12, 2022 11:15

cburgdorf changed the title ~~WIP: Remove event/emit and introduce ctx.emit that works with the Emittable trait~~ Remove event/emit and introduce ctx.emit that works with the Emittable trait Sep 12, 2022

cburgdorf requested a review from Y-Nak September 12, 2022 11:17

cburgdorf force-pushed the christoph/feat/loggable branch 3 times, most recently from d021e09 to f934794 Compare September 13, 2022 08:18

cburgdorf requested a review from g-r-a-n-t September 13, 2022 11:46

Y-Nak reviewed Sep 13, 2022

View reviewed changes

cburgdorf added 2 commits September 14, 2022 10:08

Remove event type and emit statement

b474303

Support struct field attributes in parser

e12c65f

cburgdorf force-pushed the christoph/feat/loggable branch 3 times, most recently from 4007ff9 to d8fb1e9 Compare September 14, 2022 08:59

cburgdorf added 3 commits September 14, 2022 11:01

Support events via Emittable trait

6e3d130

Refactor how generics are resolved

0ecf4e2

Update tests

a5ca513

cburgdorf force-pushed the christoph/feat/loggable branch from d8fb1e9 to a5ca513 Compare September 14, 2022 09:04

Y-Nak reviewed Sep 14, 2022

View reviewed changes

Get rid of attributes in MIR structs

b4bfa81

cburgdorf merged commit f20a677 into ethereum:master Sep 19, 2022

Y-Nak mentioned this pull request Sep 21, 2022

Panics when a struct contains a enum type #794

Closed

Y-Nak mentioned this pull request Sep 28, 2022

Remove event type and keyword #717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove event/emit and introduce ctx.emit that works with the Emittable trait #772

Remove event/emit and introduce ctx.emit that works with the Emittable trait #772

cburgdorf commented Jul 27, 2022 •

edited

cburgdorf commented Sep 12, 2022 •

edited

cburgdorf commented Sep 13, 2022

Y-Nak left a comment •

edited

cburgdorf commented Sep 14, 2022

Y-Nak left a comment

Y-Nak Sep 14, 2022

cburgdorf Sep 15, 2022

Y-Nak Sep 15, 2022

Y-Nak Sep 14, 2022

Y-Nak Sep 14, 2022

Y-Nak Sep 14, 2022

Y-Nak Sep 14, 2022

cburgdorf Sep 15, 2022

Y-Nak Sep 15, 2022

cburgdorf Sep 15, 2022

Y-Nak Sep 14, 2022

cburgdorf Sep 15, 2022

Y-Nak Sep 15, 2022 •

edited

cburgdorf Sep 15, 2022

Y-Nak Sep 15, 2022 •

edited

cburgdorf Sep 15, 2022

Y-Nak Sep 15, 2022

Y-Nak left a comment

Y-Nak Sep 14, 2022

cburgdorf Sep 15, 2022

Y-Nak Sep 14, 2022

cburgdorf Sep 15, 2022

Remove event/emit and introduce ctx.emit that works with the Emittable trait #772

Remove event/emit and introduce ctx.emit that works with the Emittable trait #772

Conversation

cburgdorf commented Jul 27, 2022 • edited

What was wrong?

How was it fixed?

cburgdorf commented Sep 12, 2022 • edited

cburgdorf commented Sep 13, 2022

Y-Nak left a comment • edited

Choose a reason for hiding this comment

cburgdorf commented Sep 14, 2022

Y-Nak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Y-Nak Sep 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Y-Nak Sep 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Y-Nak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf commented Jul 27, 2022 •

edited

cburgdorf commented Sep 12, 2022 •

edited

Y-Nak left a comment •

edited

Y-Nak Sep 15, 2022 •

edited

Y-Nak Sep 15, 2022 •

edited