Don't use symbols in Crystal::Lexer#check_macro_opening_keyword

[HertzDevil]

No description provided.

[straight-shoota]

Considering that this seems to be the only use of check_macro_opening_keyword's return value, wouldn't it be sufficient to return true/false to indicate whether to increase nesting? 🤔

[HertzDevil]

The truthiness is also used, there'll need to be at least 3 distinct return values. And I don't think repurposing Bool? for this looks good

[HertzDevil]

There might be a way

[Sija]

What's the point of replacing symbols with named tuples?

[straight-shoota]

Actually, after thinking about this again I'm not convinced about using a named tuple here just to signal three states.
IMO Bool? would be fine. Maybe not ideal for mapping the different values. Perhaps semantics would improve by renaming the method to keyword_at_macro_start_is_abstract_def?: nil means there is no keyword at macro start, false means the keyword is not abstract def and true means it is.
More fancy modeling is possible but unnecessary for the current usage.

[HertzDevil]

false would interfere with the way #lookahead works (any falsey value causes backtracking, and the other uses of #lookahead do return false), so there still need to be two distinct truthy states. I was originally going for a private enum at first but it seems even more overkill than the NamedTuple here.

[straight-shoota]

Thanks for clarification. Makes sense.

I'm not happy about using a named tuple, though. We're usually preaching their purpose is for named arguments and it's not recommended for anything else.
Maybe we can use a record instead? That's basically the same except that we need a line to define the type. But that's all.

Another alternative would be a singleton value as a second truthy state.
But expressing the same data structure as a record instead of a named tuple should be better.

[beta-ziliani]

I disagree with the idea that NamedTuple is only for named args 😅 I think of NamedTuple as an anonymous record, and I don't see a problem with it.

[straight-shoota]

@beta-ziliani I don't think we're using named tuples anywhere like that (and IMO for good reason). Instead, we have dozends of simple record types in the compiler source. We should do that here as well.

[HertzDevil]

What about:

private enum MacroKeywordState
  AbstractDef
  Other
end

[straight-shoota]

I figure a record might be more flexible should there ever be a need for additional information.
But either is fine by me.