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

time_ns cannot use variable as an argument #1123

Closed
ynadji opened this issue Feb 11, 2022 · 2 comments · Fixed by #1313
Closed

time_ns cannot use variable as an argument #1123

ynadji opened this issue Feb 11, 2022 · 2 comments · Fixed by #1313
Assignees
Labels
Enhancement Improvement of existing functionality Language

Comments

@ynadji
Copy link
Contributor

ynadji commented Feb 11, 2022

See minimal example below. If a literal is used (commenting out first two lines of Bar and uncommenting the last line) it compiles fine.

module Foo;

import spicy;

public function Bar(): time {
  local secs: uint64 = 173281732132132;
  return time_ns(secs);
  #return time_ns(173281732132132);
}
$ spicy-build -o foo foo.spicy
[error] foo.spicy:7:18: syntax error, unexpected identifier, expecting unsigned integer value or '+'
[error] spicyc: parse error

$ spicy-config --version
1.4.0-dev.174 (b64af90e)
@rsmmr rsmmr added Enhancement Improvement of existing functionality Language labels Feb 14, 2022
@rsmmr
Copy link
Member

rsmmr commented Feb 14, 2022

This is on purpose, but I admit that it's confusing. The idea is that time(<int>) actually is the Spicy literal for a constant time value, so it doesn't allow expressions inside the parentheses. But yeah, it looks like a function call / dynamic constructor, which makes it confusing.

We should improve this. think we either need to change the syntax for literals, or make the case of expressions work. Applies to a couple of other types as well.

@ynadji
Copy link
Contributor Author

ynadji commented Mar 7, 2022

Okay, that makes sense. I'd throw a +1 for making expressions work. My current use case requires some post-processing where I cannot parse all the data in a unit and would like to make time objects from uints I've "parsed."

@rsmmr rsmmr self-assigned this Dec 5, 2022
rsmmr added a commit that referenced this issue Dec 5, 2022
So far we used constructor expressions like `interval(...)` to create
*constants* of the given type, and for that we required that the
argument was a single atomic value. The result was that these
constructor expressions were really more like literals for the
corresponding types then "normal" expressions. While that's useful
internally, it's confusing for the user. This commits is beginning to
change that, using `interval` as the first instance that now supports
arbitrary expressions as its argument. In follow-up commits, we'll
port other constructor expressions over to that new syntax.

To change this without loosing a way to create actual constants (which
we need at some places), we implement this in two stages: the parser
initially turns such `interval(...)` expressions into `cast` nodes
(i.e., `cast<interval>(...)`). Such casts are already supported to
create an interval from an integer, so that just works, but it means
we don't have constants anymore. To get them back where we can, we
extend the normalizer to look at all such casts: if they are just
receiving a constant value as their argument, then it substitutes them
with a plain ``ctor::Interval`` node-—-which is what we used to have
there.

That check for "if they just receive a constant value" isn't quite
trivial in general. In our case, where we just want to go back to what
we had, it's not that difficult, but more generally it would be nice
if we had a constant folder that could compute results even for more
complex, but constant, expressions. As that's a piece that our AST
infrastructure is still missing, this commit starts going that route
by adding a new "constant folder" visitor. Right now, it's covers only
the simple cases we need here, but it's something we can extend going
forward.

Addresses #1123.

TODO: Not quite sure how to test the substitution as it's only visible
inside the AST.
@rsmmr rsmmr linked a pull request Dec 5, 2022 that will close this issue
rsmmr added a commit that referenced this issue Dec 7, 2022
…rval_ns(...)`.

So far we used constructor expressions like `interval(...)` to create
*constants* of the given type, and for that we required that the
argument was a single atomic value. The result was that these
constructor expressions were really more like literals for the
corresponding types than "normal" expressions. While that's useful
internally, it's confusing for the user. This commits is beginning to
change that, using `interval`/`interval_NS` as the first instances
that now supports arbitrary expressions as its argument. In follow-up
commits, we'll port other constructor expressions over to that new
syntax. (This also unifies HILTI & Spicy to each provide both of those
interval constructors.)

To make this change without loosing a way to create actual constants
(which we need at some places), we implement this in two stages: the
parser initially turns such `interval(...)` expressions into call
operators[1] that the types now define for each desired constructor. We
then extend the normalizer to look at all the relevant call operators:
if they are just receiving a constant value as their argument, then it
substitutes them with a plain ``Ctor`` node (``ctor::Interval`` in our
cases here).

That check for "if they just receive a constant value" isn't quite
trivial in general. In our case, where we just want to go back to what
we had, it's not that difficult, but more generally it would be nice
if we had a constant folder that could compute results even for more
complex, but constant, expressions. As that's a piece that our AST
infrastructure is still missing, this commit starts going that route
by adding a new "constant folder" visitor. Right now, it's covers only
the simple cases we need here, but it's something we can extend going
forward.

Addresses #1123.

[1] These call operators are a bit special because they actually don't
have anything to call: in `interval(42)` the `interval` isn't any kind
of actual value; we just need it for overload resolution. To make that
work, we use a `expression::Member` as the first operand, which models
a predefined ID. Normally `Member` is just for field lookups in, e.g.,
structs, but it works just as well elsewhere.
rsmmr added a commit that referenced this issue Dec 9, 2022
…rval_ns(...)`.

So far we used constructor expressions like `interval(...)` to create
*constants* of the given type, and for that we required that the
argument was a single atomic value. The result was that these
constructor expressions were really more like literals for the
corresponding types than "normal" expressions. While that's useful
internally, it's confusing for the user. This commits is beginning to
change that, using `interval`/`interval_NS` as the first instances
that now supports arbitrary expressions as its argument. In follow-up
commits, we'll port other constructor expressions over to that new
syntax. (This also unifies HILTI & Spicy to each provide both of those
interval constructors.)

To make this change without loosing a way to create actual constants
(which we need at some places), we implement this in two stages: the
parser initially turns such `interval(...)` expressions into call
operators[1] that the types now define for each desired constructor. We
then extend the normalizer to look at all the relevant call operators:
if they are just receiving a constant value as their argument, then it
substitutes them with a plain ``Ctor`` node (``ctor::Interval`` in our
cases here).

That check for "if they just receive a constant value" isn't quite
trivial in general. In our case, where we just want to go back to what
we had, it's not that difficult, but more generally it would be nice
if we had a constant folder that could compute results even for more
complex, but constant, expressions. As that's a piece that our AST
infrastructure is still missing, this commit starts going that route
by adding a new "constant folder" visitor. Right now, it's covers only
the simple cases we need here, but it's something we can extend going
forward.

Addresses #1123.

[1] These call operators are a bit special because they actually don't
have anything to call: in `interval(42)` the `interval` isn't any kind
of actual value; we just need it for overload resolution. To make that
work, we use a `expression::Member` as the first operand, which models
a predefined ID. Normally `Member` is just for field lookups in, e.g.,
structs, but it works just as well elsewhere.
rsmmr added a commit that referenced this issue Jan 11, 2023
…rval_ns(...)`.

So far we used constructor expressions like `interval(...)` to create
*constants* of the given type, and for that we required that the
argument was a single atomic value. The result was that these
constructor expressions were really more like literals for the
corresponding types than "normal" expressions. While that's useful
internally, it's confusing for the user. This commits is beginning to
change that, using `interval`/`interval_NS` as the first instances
that now supports arbitrary expressions as its argument. In follow-up
commits, we'll port other constructor expressions over to that new
syntax. (This also unifies HILTI & Spicy to each provide both of those
interval constructors.)

To make this change without loosing a way to create actual constants
(which we need at some places), we implement this in two stages: the
parser initially turns such `interval(...)` expressions into call
operators[1] that the types now define for each desired constructor. We
then extend the normalizer to look at all the relevant call operators:
if they are just receiving a constant value as their argument, then it
substitutes them with a plain ``Ctor`` node (``ctor::Interval`` in our
cases here).

That check for "if they just receive a constant value" isn't quite
trivial in general. In our case, where we just want to go back to what
we had, it's not that difficult, but more generally it would be nice
if we had a constant folder that could compute results even for more
complex, but constant, expressions. As that's a piece that our AST
infrastructure is still missing, this commit starts going that route
by adding a new "constant folder" visitor. Right now, it's covers only
the simple cases we need here, but it's something we can extend going
forward.

Addresses #1123.

[1] These call operators are a bit special because they actually don't
have anything to call: in `interval(42)` the `interval` isn't any kind
of actual value; we just need it for overload resolution. To make that
work, we use a `expression::Member` as the first operand, which models
a predefined ID. Normally `Member` is just for field lookups in, e.g.,
structs, but it works just as well elsewhere.
rsmmr added a commit that referenced this issue Jan 27, 2023
…ions'

* origin/topic/robin/gh-1123-ctor-expressions: (23 commits)
  Change SafeInt repository over to a fork including a fix.
  Fix issue in Bison grammar.
  Suppress GCC warnings.
  Fix a couple of unit tests for Alpine.
  Fix clang-tidy warnings.
  Fix printing of ID scopes.
  Fix printing of enum constants.
  Add boolean folding.
  Outsource an integer range check to SafeInt.
  Suppress clang-tidy warnings.
  Remove debugging output from integer range checks.
  Add test to check substitution of ctor nodes.
  Update printer to output type constructors.
  Move ctor replacement from normalizer into constant folder.
  Add test exercising various integer out-of-range situations.
  Add general constant folding.
  Rework integer out-of-range checking.
  Move negation out of parser.
  Address review feedback.
  Port `port` ctor expression to new model.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement of existing functionality Language
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants