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

External hook implementation for fields/units and error messages #1465

Closed
awelzel opened this issue Jun 22, 2023 · 1 comment
Closed

External hook implementation for fields/units and error messages #1465

awelzel opened this issue Jun 22, 2023 · 1 comment
Assignees

Comments

@awelzel
Copy link
Contributor

awelzel commented Jun 22, 2023

@J-Gras found that the package template showed on Module::Unit { ... } usages in a comment and fixed it to explicitly use a ::%done as without that it's not functional: zeek/package-template@c429a20

Two topics related to that:

  1. Should implicit %done work for external hooks on units as previously shown in the template, or was that an oversight? For unit fields %done is not needed, though that may be mixing different concepts that only on first sight seem related.

  2. The error messages produced for hooks that have an extra %done on unit fields or a a missing %done on units fields are not helpful for diagnosing the issue.

Test files:

# test_mod.spicy
module test_mod;

public type Y = unit {
  y: bytes &size=1;
};
public type X = unit {
  x: bytes &size=1;
  y: 
# test_mod.spicy 
module zee_test_mod;

import test_mod;

# works
on test_mod::X::%done { }

# error: cannot use module 'test_mod' as an ID - could we say `%done` missing, or use `%done` implicitly?
# on test_mod::X { }

# works - is this implicit %done on the field or something else?
on test_mod::X::x { }

# error: unknown ID 'test_mod::X::b' - this is an extra %done where there shouldn't be one.
# on test_mod::X::b::%done { }

# works - is this implicit %done on the field?
on test_mod::X::y { }

# error: unknown ID 'test_mod::X::y' - same as for `::b` but with a unit type.
# on test_mod::X::y::%done { }
@rsmmr
Copy link
Member

rsmmr commented Jun 26, 2023

  1. Should implicit %done work for external hooks on units as previously shown in the template, or was that an oversight?

Yes, it's meant to work. I think we ran into this issue before somewhere but looks like we don't track it in a ticket yet.

a missing %done on units fields are not helpful for diagnosing the issue.

One clarification: %done works only on units, not on unit fields. I guess one could say %done is always implicit on fields, but that we don't have %init on fields either, so that wouldn't be very consistent. But we can certainly improve error messaging here.

@rsmmr rsmmr self-assigned this Jul 5, 2023
rsmmr added a commit that referenced this issue Jul 5, 2023
Assuming `Foo::X` is a unit type, these two are now equivalent:

    on Foo::X::%done   { }
    on Foo::X          { }

Addresses #1465.
rsmmr added a commit that referenced this issue Jul 5, 2023
Assuming `Foo::X` is a unit type, these two are now equivalent:

    on Foo::X::%done   { }
    on Foo::X          { }

Addresses #1465.
rsmmr added a commit that referenced this issue Jul 6, 2023
Assuming `Foo::X` is a unit type, these two are now equivalent:

    on Foo::X::%done   { }
    on Foo::X          { }

Addresses #1465.
@rsmmr rsmmr closed this as completed in 7e54637 Jul 7, 2023
rsmmr added a commit that referenced this issue Jul 7, 2023
* origin/topic/robin/gh-1465-implicit-done:
  Produce better error message when `%XXX` hook is used on a unit field.
  Support skipping explicit `%done` in external hooks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants