Skip to content

Conversation

@ydah
Copy link
Member

@ydah ydah commented Feb 4, 2025

❯ exe/lrama --define=foo sample/calc.y
wrong array length at 0 (expected 2, was 1)

This is occurring and has been addressed below:

@define = define.map {|d| d.split('=') }.to_h

…dling

`@options.define` is expect to Array so default value is also Array not Hash.
@yui-knk
Copy link
Collaborator

yui-knk commented Feb 4, 2025

I'm not sure why steep didn't warn however please update type declaration too.

attr_accessor define: Hash[String, String]

@yui-knk
Copy link
Collaborator

yui-knk commented Feb 4, 2025

[Note] It seems steep 1.9.3 doesn't warn on array assignment to Hash type instance variable:

# code
@define = []

# rbs
attr_accessor define: Hash[String, String]
$ bundle exec steep check
# Type checking files:

......................................................................................................................

No type error detected. 🫖

but it warns on hash assignment to Array type instance variable:

# code
@define = {}

# rbs
attr_accessor define: Array[String]
$ bundle exec steep check
# Type checking files:

.....................................................................F................................................

lib/lrama/options.rb:15:16: [warning] Empty hash doesn't have type annotation
│ Diagnostic ID: Ruby::UnannotatedEmptyCollection

└       @define = {}
                  ~~

Detected 1 problem from 1 file

@junk0612
Copy link
Collaborator

junk0612 commented Feb 4, 2025

This change alone is probably not sufficient, and changes are needed, for example, in the location of the assignment at below.

lrama/parser.y

Line 28 in 7fa74d8

| "%define" variable value { @grammar.define[val[1].s_value] = val[2]&.s_value }

By the way, I think Hash is appropriate for define because it is written in the grammar file as %define lr.type ielr or from the command line as lrama -D lr.type=ielr, but why change it to Array?

@ydah
Copy link
Member Author

ydah commented Feb 4, 2025

This change alone is probably not sufficient, and changes are needed, for example, in the location of the assignment at below.

Changes there will be made separately. Certainly as it is now, if a value is missing, it will result in an error.

By the way, I think Hash is appropriate for define because it is written in the grammar file as %define lr.type ielr or from the command line as lrama -D lr.type=ielr, but why change it to Array?

I didn't mean to change the @define to Array in the first place. Because, below, I am converting from Array to Hash. It looks like the intended code.

@define = define.map {|d| d.split('=') }.to_h

This PR simply changes the default value from Hash to Array.
Because the following Option is received in Array.

o.on('-D', '--define=NAME[=VALUE]', Array, "similar to '%define NAME VALUE'") {|v| @options.define = v }

If I understand something incorrectly, please point it out to me.

```
❯ exe/lrama --define=foo sample/calc.y
wrong array length at 0 (expected 2, was 1)
```

This is occurring and has been addressed below:
https://github.com/ruby/lrama/blob/7fa74d84a68b3632950b969d29ccf012a0ea4a42/lib/lrama/grammar.rb#L60
@ydah
Copy link
Member Author

ydah commented Feb 5, 2025

I changed it back to Hash to avoid the error 569f568
And moved to rbs-inline style type definition 4bd35c0

@ydah ydah changed the title Change @define from hash to array and add tests for define option handling Fix an error when specify --define=foo Feb 5, 2025
@junk0612
Copy link
Collaborator

junk0612 commented Feb 5, 2025

I didn't mean to change the @define to Array in the first place. Because, below, I am converting from Array to Hash. It looks like the intended code.

I see, indeed. My understanding was wrong.

@ydah ydah merged commit 58b28b9 into ruby:master Feb 5, 2025
22 checks passed
@ydah ydah deleted the change-option branch February 5, 2025 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants