Feature #14506: Possible bug in cmdarg_stack

• Status: Closed
• Assignee: nobu (Nobuyoshi Nakada)
• Target version: -

[ibylich (Ilya Bylich)]

cmdarg stack is a stack of bytes that has 1 on top of it when the parser is in the command mode.

But for this code:

a b(c d), "x" do end

And this line in parse.y:

> SHOW_BITSTACK(p->cmdarg_stack, "BEFORE kDO_BLOCK");
if (CMDARG_P() && !IS_lex_state_for(state, EXPR_CMDARG))
  return keyword_do_block;

It prints:

$ dev-ruby -vye 'a b(c d), "x" do end' | grep -E "kDO_BLOCK"
BEFORE kDO_BLOCK: 10 at line 7461

So CMDARG_P is false. I understand that 'keyword_do_block" is handled later in the "if (IS_lex_state_for(state, (EXPR_BEG | EXPR_ENDARG)))" section, but is it possible to change parser to rely only on cmdarg stack?

[ibylich (Ilya Bylich)]

I've sent a PR https://github.com/ruby/ruby/pull/1823

[mame (Yusuke Endoh)]

• Status changed from Open to Assigned
• Assignee set to nobu (Nobuyoshi Nakada)

I think that this is not a bug, and that your patch is for refactoring. Am I right?

Please clarify your motivation if any. I understand that you want this change for ease of backporting from parser.y to the parser gem. I'm neutral to this.

Anyway, your patch looks good to me as a refactoring. Nobu, what do you think?

[ibylich (Ilya Bylich)]

This patch does not introduce any logical changes (at least I don't expect any).

mame (Yusuke Endoh) wrote:

I think that this is not a bug, and that your patch is for refactoring. Am I right?

Yes, exactly.

mame (Yusuke Endoh) wrote:

Please clarify your motivation if any. I understand that you want this change for ease of backporting from parser.y to the parser gem. I'm neutral to this.

Well, MRI's parser can rely only on cmdarg to recognize keyword_do/keyword_do_block, but for example above it doesn't. Instead, it transitions into dual state (which, from what I understand, works like a flag). Parser gem uses Ragel for lexing and Ragel doesn't support dual states.
The motivation of this patch is to refactor and simplify this place in MRI to allow backporting of this feature "as is" to the parser gem.

[nobu (Nobuyoshi Nakada)]

Seems nice.
Maybe, lambda and brace_body parts aren't needed?

And, just curiosity, what do you mean by "dual state", cmdarg and lex_state?

[ibylich (Ilya Bylich)]

nobu (Nobuyoshi Nakada) wrote:

Seems nice.
Maybe, lambda and brace_body parts aren't needed?

Maybe, I didn't have a chance to test it. Would you like me to check and revert this part?

nobu (Nobuyoshi Nakada) wrote:

And, just curiosity, what do you mean by "dual state", cmdarg and lex_state?

No, when lexer comes to the "do" token it has a state "EXPR_END|EXPR_ENDARG". I understand that it's a bitmask, but from what I see "EXPR_ENDARG" flag is needed only for emitting "do" as "keyword_do_block" (while originally cmdarg is designed to handle it and moreover, it can handle it on its own). Ragel doesn't support such states, in the parser gem it can be either EXPR_END or EXPR_ENDARG, but not both at the same time. That's why I have troubles with backporting it.

For MRI it's just a refactoring, for the parser gem it's (from what I see) the only way to backport it and keep semantical structure.

[nobu (Nobuyoshi Nakada)]

• Tracker changed from Bug to Feature
• ruby -v deleted (ruby 2.6.0dev (2018-02-19 trunk 62484) [x86_64-darwin17])
• Backport deleted (2.3: UNKNOWN, 2.4: UNKNOWN, 2.5: UNKNOWN)

ibylich (Ilya Bylich) wrote:

Maybe, lambda and brace_body parts aren't needed?

Maybe, I didn't have a chance to test it. Would you like me to check and revert this part?

No, just confirmed.

No, when lexer comes to the "do" token it has a state "EXPR_END|EXPR_ENDARG". I understand that it's a bitmask, but from what I see "EXPR_ENDARG" flag is needed only for emitting "do" as "keyword_do_block" (while originally cmdarg is designed to handle it and moreover, it can handle it on its own). Ragel doesn't support such states, in the parser gem it can be either EXPR_END or EXPR_ENDARG, but not both at the same time. That's why I have troubles with backporting it.

Thank you for clarification.

[nobu (Nobuyoshi Nakada)]

• Status changed from Assigned to Closed

Applied in changeset trunk|r62528.

---

Fix CMDARG manipulation

• parse.y: Fix CMDARG manipulation. Use CMDARG_P to identify
  keyword_do/keyword_do_block. [Feature #14506] [Fix GH-1823]

From: Ilya Bylich [e-mail]