JMSLab / xtevent

Stata package -xtevent-
MIT License
43 stars 12 forks source link

"*" in syntax leads to unexpected lack of syntax checks #193

Closed jorpppp closed 3 months ago

jorpppp commented 3 months ago

Consider this MWE

cap program drop mytest

program define mytest

syntax, [norm(integer -1 ) *]

di "`norm'"
di "`options'"

end

mytest, norm(-1)
mytest, norm(abc)

Because of the * in the syntax, Stata is not returning an invalid syntax error for mytest, norm(abc) even though it is not the right syntax because norm should be a string. Instead, it is using the default value for norm and passing the incorrectly-syntaxed norm(abc) option as an extra option in options.

This means that Stata is not properly checking for syntax errors in some places. This has not generated problems for us so far, but it might. We may want to change some of the internal usage of "*" to avoid this.

jorpppp commented 3 months ago

I asked about this in Statalist: https://www.statalist.org/forums/forum/general-stata-discussion/general/1753201-using-in-syntax-leads-to-unexpected-lack-of-syntax-checks

jorpppp commented 3 months ago

From Statalist: https://www.statalist.org/forums/forum/general-stata-discussion/general/1753201-using-in-syntax-leads-to-unexpected-lack-of-syntax-checks

We can solve this behavior using this syntax:

program define mytest  
  syntax [ , NORM(numlist integer max=1) * ]
  if ("`norm'" == "") local norm -1
  di "`norm'"  
  di "`options'"
end

mytest, norm(-1)  
mytest, norm(abc)

So we should define default values in a second step to make Stata do syntax checks in all options.

jorpppp commented 3 months ago

In https://github.com/JMSLab/xtevent/commit/5c18eab182a133dd472791d32bc27c23fadf1d72 I changed the syntax in a couple of places to ensure syntax checks. The changes were small, they only affected the norm and suptreps options.

I also acknowledged Daniel Klein who answered this on Statalist.

jorpppp commented 3 months ago

Summary: In this issue we changed syntax to ensure syntax checks even when passing unnamed options.

Thread continues in #212