consistency checking for attributes

Attributes currently receive essentially no consistency checking.
For example, executing this script:

  global a: count
    &default = 10
    &default = 9
    &optional
    &log
    &add_func = function(d: double, t: time): count { return 3; };
  print a;

simply results in:

error in /Users/vern/tmp/attr-type-check.bro, line 7: value used but not set (a)

I'm planning to add basic consistency checking, which will look for
(1) attributes that are repeated (which doesn't appear to be meaningful for
any of them) and (2) attributes that don't make sense in a given context,
like the ones listed above.

I'm thinking of implementing this as an internal table of meta-attributes,
i.e., each attribute type, like ATTR_OPTIONAL, will have its own attributes
like whether it requires a record context, only makes sense for aggregates,
etc. Here are the ones that come to mind, based on looking at the attributes
at Attributes — Book of Zeek (v5.0.0) with examples
in parens:

  applies to global variable (&redef)
    to global type (&redef)
    to event handler (&priority)
    to record field (&log)
    to indexed type (&default)
    to global indexed type (&add_func)
    to type with expirable entries (&expire_func)
    to a file (&rotate_interval)

Any feedback?

    Vern

I'm thinking of implementing this as an internal table of meta-attributes,
i.e., each attribute type, like ATTR_OPTIONAL, will have its own attributes
...

Looking into this further, it appears that the basic issue is that while
Attributes::CheckAttr already tries to make sure that a given attribute
makes sense, its checking is deficient. Some of this might be because it
lacks information about whether the attribute is appearing in the context
of a type or a variable. Also, Attributes::AddAttr already checks for
repeated attributes, but it just silently replaces them. This works okay for
a situation where a type is being refined by introducing updated attributes,
but doesn't make sense when all of the attributes are presented together
(like in my example); again, it needs more context to figure that out.

    Vern

Sounds good to me.

Robin

Sounds good and I like it. To be honest I’m not even sure if the behavior is defined right now, i.e. if the later value will overwrite the first one.

Do you want to error out when two &defaults are found or overwrite the first with the second one?

To be honest I'm not even sure if the behavior
is defined right now, i.e. if the later value will overwrite the first one.

I don't think it's defined. Looking at the code, the later one will
indeed overwrite the first one.

Do you want to error out when two &defaults are found or overwrite the
first with the second one?

My thinking is that listing them together should be an error. I don't
see why it would be legitimate for this to occur, and it seems like a
mistake that could be made if there's a lengthy list of attributes.

    Vern

Yeah, error out and let there be no surprises.