Bug #21994
open
If there is a local variable `foo`, calls to a method `foo` with a regexp literal as first argument is always a SyntaxError without parentheses
Description
Quick version:¶
ruby -e 'p /hello/' # => /hello/ (warning)
ruby -e 'p = 1; p /hello/' # syntax error
ruby -e 'p %r/hello/' # => /hello/ (no warning)
ruby -e 'p = 1; p %r/hello/' # syntax error
Context¶
In Rouge, our lexing DSL originally looked like this:
rule /some_regex/, Some::TokenType, :next_state
rule /some_other_regex/ do |match|
# custom action
end
In a DSL context like this, I feel that using parentheses for every rule definition is a hassle and looks bad, so we leave them off, as is convention.
Unfortunately, this warns:
warning: ambiguous `/`; wrap regexp in parentheses or add a space after `/` operator
Some time ago, to reduce warning-spam on users who run with -w, we transitioned all of these to use %r/.../ or %r(...) syntax, which does not warn.
The issue here I think is that both cases are equivalently ambiguous in some sense, which becomes apparent with the local-scope-dependent behaviour of the parser (putting aside that dependency on local variable scope makes the Ruby syntax highlighting of Rouge more or less impossible to do correctly without a full parse). If there is a local variable named rule introduced at any point, even several block scopes above, the entire file errors out in a very confusing manner.
At the very least, I feel that the warnings here are inconsistent. If both cases truly are ambiguous and dependent on local variable scope, perhaps it would be better to warn on both - though that would make using a regex literal as a first argument always require parentheses to avoid warnings, which would make my DSL much harder. I'm honestly not entirely sure what a good solution would be, but I would like to open a discussion on the topic. Perhaps philosophically related to the other bug I have open ( #21870 ) - is this truly worthy of a warning, or should this be a linter concern?
Updated by Earlopain (Earlopain _) 1 day ago
- Related to Feature #20922: Should not we omit parentheses in assert calls? added
Updated by jneen (Jeanine Adkisson) 1 day ago
· Edited
I should say that I think the worst solution to this would be to warn in both cases. It would effectively require us to rewrite our DSL across every rule for every language we support. My ideal world is one where we could go back to using rule /regex/ do ... with no additional specification, or perhaps to warn in the division/modulo case instead. I would very much like to not have to turn off warnings while loading.
EDIT: a new warning would also affect more common patterns like Set.new %w(...).
Updated by byroot (Jean Boussier) 1 day ago
I know very little about parser, so perhaps what I'm about to suggest is entirely impossible, but could we refine that warning as to only trigger it if the other interpretation would be valid syntax?
e.g.:
-
p -1- Both
p(-1)andp - 1are valid so the warning makes sense.
- Both
-
p -1, 2,(p - 1), 2isn't valid ruby, onlyp(-1, 2)is so the warning could not be triggered.
I suspect there are considerations I don't know about though.
Updated by tompng (tomoya ishida) about 11 hours ago
I think checking code validness is hard for the code below.
Warning check is triggered at every line, each check needs to parse till the bottom.
a -1 => b # only a(-1 => b) is valid, (a - 1) => b is invalid at the `2 => 3` position
b -1 => c # only b(-1 => c) is valid
c -1 => d # only c(-1 => d) is valid
d -1 => e, 2 => 3 # invalid if local variable d is defined
Updated by jneen (Jeanine Adkisson) about 8 hours ago
· Edited
I think checking code validness is hard
I agree, since this check happens during lexical analysis, rather than parsing. It is a bit mind-boggling to me that lexical analysis depends on local variable scope, but it's what it is.
I tend to feel that the infix operator case is nicer and easier for the end-user to fix than the method call case. a -1 is already not good style for subtraction - I would ask a contributor to format this either a-1 or a - 1 if that was in a pull request. However, rule /regex/ do ... I would consider to be good style, if it didn't warn. And I do feel it is fair to ask for some way to call a method with a regex as first argument without parentheses, even if it's %r/.../ syntax.