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

optional<bytes> differs between struct and tuple only as a parameter #1143

Closed
ynadji opened this issue Feb 24, 2022 · 3 comments · Fixed by #1259
Closed

optional<bytes> differs between struct and tuple only as a parameter #1143

ynadji opened this issue Feb 24, 2022 · 3 comments · Fixed by #1259
Labels
Bug Something isn't working

Comments

@ynadji
Copy link
Contributor

ynadji commented Feb 24, 2022

This came up when trying to convert a struct to a tuple in Spicy to pass to Zeek. This same construct worked with uint64 but failed with a cryptic error when I tried the same thing with bytes. It's possible other types would meet the same fate. Code and error follow:

module Foo;

type Bar = struct {
  wutang: bytes &optional;
};

type BarTuple = tuple<
  wutang: optional<bytes>
>;

public function make_bar_tuple(bar: Bar): BarTuple {
  local nope: optional<bytes>;
  return (
    bar?.wutang ? optional(bar.wutang) : nope
  );
}
¡ spicy-build -o foo foo.spicy
[error] foo.spicy:14:5: types of alternatives do not match in ternary expression (optional<bytes> vs. optional<bytes>)
[error] spicyc: aborting after errors

However, if bar is instead declared locally, it compiles without issue:

module Foo;

type Bar = struct {
  wutang: bytes &optional;
};

type BarTuple = tuple<
  wutang: optional<bytes>
>;

public function make_bar_tuple(): BarTuple {
  local bar: Bar;
  local nope: optional<bytes>;
  return tuple(bar?.wutang ? optional(bar.wutang) : nope);
}

(Fixed code to actually return tuple instead instead of optional - Robin).

@ynadji ynadji changed the title optional<bytes> differs between struct and tuple optional<bytes> differs between struct and tuple only as a parameter Feb 24, 2022
@bbannier bbannier added the Bug Something isn't working label Feb 25, 2022
@bbannier bbannier self-assigned this Apr 21, 2022
@bbannier
Copy link
Member

bbannier commented Apr 21, 2022

The issue here is that while type comparison initially invokes sameExceptForConstness two different type nodes without typeID or cxxID were created so we forward to the node equality check Type::isEqual https://github.com/zeek/spicy/blob/d525ad3038724e8543a8387372bb29e21c4461f6/hilti/toolchain/include/ast/type.h#L368= which ultimately calls operator==(const Type&, const Type&) (which takes constness into account). The proper fix is probably to also add support for sameExceptForConstness on the node level.

@bbannier
Copy link
Member

bbannier commented May 9, 2022

It looks like no quick fix is possible here. Instead of pushing sameExceptForConstness the proper fix is probably to instead move constness out of the type information, and then handle it explicitly where needed.

Unassigning myself for now.

@bbannier bbannier removed their assignment May 9, 2022
@rsmmr
Copy link
Member

rsmmr commented Aug 10, 2022

(deleted my previous comment as it wasn't right.)

rsmmr added a commit that referenced this issue Aug 10, 2022
…er types.

There are some situations where that's fine to coerce, so adding that.
Also adding a coercion for ternary expressions that coerces the 2dn
expression into the type of the 1st (which we already assume provides
the result type).

Closes #1143.
Closes #1220.
@rsmmr rsmmr closed this as completed in 14478b2 Aug 18, 2022
rsmmr added a commit that referenced this issue Aug 18, 2022
…rce'

* origin/topic/robin/gh-1143-optional-coerce:
  Add coercion on assignment for optionals that only differ in constness of their inner types.
bbannier pushed a commit that referenced this issue Aug 22, 2022
…s of their inner types.

Also adding a coercion for ternary expressions that coerces the 2dn
expression into the type of the 1st (which we already assume provides
the result type).

Closes #1143.
Closes #1220.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants