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

Function arguments should not be allowed to shadow #2690

Closed
VMatthijs opened this issue Nov 15, 2018 · 2 comments
Closed

Function arguments should not be allowed to shadow #2690

VMatthijs opened this issue Nov 15, 2018 · 2 comments

Comments

@VMatthijs
Copy link
Member

Summary:

We have intentionally disallowed shadowing (masking) of variable names from an outer scope by ones in an inner scope almost everywhere in the language, so as not to confuse novice programmers. One place where we have not done so is for function arguments. I think it should be enforced there as well.

Description:

Function arguments can currently clash with math library primitive names or with previously defined function names.

Reproducible Steps:

The following example model is currently accepted by the Stan parser:

functions {
  void foo(){}

  real relative_diff(real x, real y, real max, real min, int foo) {
    real abs_diff;
    real avg_scale;
    abs_diff = fabs(x - y);
    avg_scale = (fabs(x) + fabs(y)) / 2;
    if ((abs_diff / avg_scale) > max)
      reject("user-specified rejection, difference above ",max," x:",x," y:",y);
    if ((abs_diff / avg_scale) < min)
      reject("user-specified rejection, difference below ",min," x:",x," y:",y);
    return abs_diff / avg_scale;
  }    
}
transformed data {
  real a =  -9.0;
  real b = -1.0;
  real mx = 1.2;
  real mn = 1.1;
}
parameters {
  real y;
}
model {
  real c;
  c = relative_diff(a,b,mx,mn, 1);
  y ~ normal(0,1);
}

Current Output:

The parser does not complain.

Expected Output:

I would expect it to complain for two different reasons: 1. the argument max clashes with a math library function name; 2. the argument foo clashes with the name of a previously defined function.

Current Version:

v2.18.0

@bob-carpenter
Copy link
Contributor

We can eliminate the max case (keyword), but not foo (masking). The problem is that if we have libraries of functions, we don't want their use or even well-formedness to depend on what calls them.

@rok-cesnovar
Copy link
Member

fixed in stanc3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants