stan-dev / stan

Stan development repository. The master branch contains the current release. The develop branch contains the latest stable development. See the Developer Process Wiki for details.
https://mc-stan.org
BSD 3-Clause "New" or "Revised" License
2.61k stars 369 forks source link

Operator precedence problem #2711

Closed VMatthijs closed 4 years ago

VMatthijs commented 5 years ago

Summary:

There is an inconsistency between the operator precedence as implemented in stanc2 and as described in the manual.

Description:

The manual describes that .* binds more tightly than *. stanc2 seems to bind * more tightly.

Reproducible Steps:

Try to compile

model {
  print(1.0 .* [1., 2.]');
}

Get error

SYNTAX ERROR, MESSAGE(S) FROM PARSER:
No matches for:

  real .* vector

Available argument signatures for operator.*:

  vector .* vector
  row_vector .* row_vector
  matrix .* matrix

Expression is ill formed.
 error in 'stan/src/test/test-models/good/empty.stan' at line 2, column 25
  -------------------------------------------------
     1: model {
     2:   print(1.0 .* [1., 2.]');
                                ^
     3: }
  -------------------------------------------------

So clearly, the signatures for . are vector . vector row_vector . row_vector matrix . matrix .

Now, if we try to compile

model {
  print([3., 4.]' * 1.0 .* [1., 2.]');
}

we would expect that to fail. Indeed, it should be parsed as

model {
  print([3., 4.]' * (1.0 .* [1., 2.]'));
}

according to the manual. However, it parses just fine and prints [3, 8] when it runs. This suggests strongly to me that it has parsed the model as

model {
  print(([3., 4.]' * 1.0) .* [1., 2.]');
}

instead.

Additional Information:

Rstanarm is currently exploiting this bug.

This also makes me worried about the other stated operator precedences. Has this been resulting in incorrect generated code?

Current Version:

v2.18.0

VMatthijs commented 5 years ago

@bob-carpenter , thoughts on this? Is the manual wrong or the implementation?

bob-carpenter commented 5 years ago

I would think they should have the same precedence and make both and . left associative. That way, the example

[3, 4] 2 . [5, 6]

will be parsed as

([3, 4] 2) . [5, 6]

On Dec 14, 2018, at 10:27 AM, Matthijs Vákár notifications@github.com wrote:

@bob-carpenter , thoughts on this? Is the manual wrong or the implementation?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

VMatthijs commented 5 years ago

Thanks! So the docs should be changed as well then? What about ./? Should that also have the same precedence?

bob-carpenter commented 5 years ago

Yes, ./ and / should have same precedence (and presumably the same precedence as . and ).

On Dec 14, 2018, at 12:16 PM, Matthijs Vákár notifications@github.com wrote:

Thanks! So the docs should be changed as well then? What about ./? Should that also have the same precedence?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mcol commented 4 years ago

Fixed in stanc3 and in docs (https://github.com/stan-dev/docs/pull/65).