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

Cannot pass context as inout parameter #1170

Closed
bbannier opened this issue May 5, 2022 · 1 comment · Fixed by #1260
Closed

Cannot pass context as inout parameter #1170

bbannier opened this issue May 5, 2022 · 1 comment · Fixed by #1260
Assignees
Milestone

Comments

@bbannier
Copy link
Member

bbannier commented May 5, 2022

In spicy-1.3.0 it was possible to pass a context as an inout parameter. This allowed users to work around #1059 in a supposedly forward-compatible way. That workaround is broken with spicy-1.4.1.

# analyzer.spicy
module foo;

type Ctx = unit { };

type X = unit(inout ctx: Ctx) { };

public type Y = unit {
	%context = Ctx;
	: X(self.context());
};
$ spicyc -dj analyzer.spicy
/private/var/folders/ht/vtb0rkbd4ws15vx02xdb84zw0000gn/T/foo_203c58e6432c1c5-ea163d6951a7f53a.cc:187:25: error: no matching
      conversion for functional-style cast from 'ValueReference<__hlt::foo::Ctx>' to '__hlt::foo::X'
    __transient_anon = (__hlt::foo::X((*__self).__context.derefAsValue()));
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/private/var/folders/ht/vtb0rkbd4ws15vx02xdb84zw0000gn/T/foo_203c58e6432c1c5-ea163d6951a7f53a.cc:43:9: note: candidate
      constructor not viable: no known conversion from 'ValueReference<__hlt::foo::Ctx>' to 'const __hlt::foo::X' for 1st
      argument
        X(const X&) = default;
        ^
/private/var/folders/ht/vtb0rkbd4ws15vx02xdb84zw0000gn/T/foo_203c58e6432c1c5-ea163d6951a7f53a.cc:44:9: note: candidate
      constructor not viable: no known conversion from 'ValueReference<__hlt::foo::Ctx>' to '__hlt::foo::X' for 1st argument
        X(X&&) = default;
        ^
/private/var/folders/ht/vtb0rkbd4ws15vx02xdb84zw0000gn/T/foo_203c58e6432c1c5-ea163d6951a7f53a.cc:119:15: note: candidate
      constructor not viable: expects an l-value for 1st argument
    inline X::X(::hilti::rt::ValueReference<__hlt::foo::Ctx>& __p_ctx) : __p_ctx(std::move(__p_ctx)) {
              ^
/private/var/folders/ht/vtb0rkbd4ws15vx02xdb84zw0000gn/T/foo_203c58e6432c1c5-ea163d6951a7f53a.cc:116:15: note: candidate
      constructor not viable: requires 0 arguments, but 1 was provided
    inline X::X() {
              ^
1 error generated.
[error] spicyc: JIT compilation failed

Bisecting Spicy points to 2905477 as the commit where this broke.

@bbannier bbannier added this to the spicy-1.5 milestone May 5, 2022
@bbannier
Copy link
Member Author

bbannier commented May 5, 2022

As a workaround one can pass the context as a reference:

type X = unit(inout ctx: Ctx&) { };

@rsmmr rsmmr self-assigned this Aug 11, 2022
@rsmmr rsmmr closed this as completed in e8933a6 Aug 11, 2022
@rsmmr rsmmr reopened this Aug 11, 2022
rsmmr added a commit that referenced this issue Aug 11, 2022
This is an attempt at fixing #1170, and maybe further issues, too
(potentially even #674, not sure yet).

The problem is that `inout` parameters require a LHS value that can be
passed by reference, but our C++ code generator currently has no way
of telling what type a C++-side expression ends up having; and hence
can't turn a RHS into a LHS when needed. This is an attempt at
providing that information: `cxx::Expression` now tracks whether it
refers to a LHS, and the code generator creates additional temporaries
at places where it needs an LHS but only got a RHS.

The current state already works to fix #1170, but I haven't checked
further for any new problems it may introduce. If this generally
works, I'll also need to go through the code generator and make sure
all places producing LHSs are indeed marked as such, so that we avoid
introducing unnecessary temporaries. Finally, there's some special
paths in the code already working around the very same problem by
creating LHSs manually in some cases where this has come up in the
past. Those could go out now.
@rsmmr rsmmr linked a pull request Aug 18, 2022 that will close this issue
rsmmr added a commit that referenced this issue Aug 18, 2022
* origin/topic/robin/gh-1170-inout:
  Fix contexts not allowing being passed `inout`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants