Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow duplicate fun parameter names #11967

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Apr 4, 2022

The following fails during codegen:

fun foo(x : Int32, x : Int32)
end

foo 1, 2
Module validation failed: Store operand must be a pointer.
  store i32 %x1, i32 %x, !dbg !729
 (Exception)
  from /crystal/src/int.cr:541:9 in 'finish'
  from /crystal/src/compiler/crystal/codegen/codegen.cr:71:7 in 'codegen'
  from /crystal/src/compiler/crystal/compiler.cr:173:16 in 'compile'
  from /crystal/src/compiler/crystal/command.cr:220:5 in 'run_command'
  from /crystal/src/compiler/crystal/command.cr:127:7 in '__crystal_main'
  from /crystal/src/crystal/main.cr:115:5 in 'main'
  from src/env/__libc_start_main.c:94:2 in 'libc_start_main_stage2'

Having two xs is not allowed for defs anyway, so it makes sense the same should apply to funs, including lib funs which have no body, as named argument passing only behaves properly when all the parameters have unique names.

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:parser labels Apr 4, 2022
@HertzDevil HertzDevil marked this pull request as draft April 4, 2022 15:57
@HertzDevil HertzDevil marked this pull request as ready for review April 4, 2022 16:10
Comment on lines +5624 to +5628
args.each do |arg|
if arg.name == arg_name
raise "duplicated fun parameter name: #{arg_name}", arg_location
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if using each is an optimization but isn't there a built in method for this?

Suggested change
args.each do |arg|
if arg.name == arg_name
raise "duplicated fun parameter name: #{arg_name}", arg_location
end
end
if args.includes?(arg_name)
raise "duplicated fun parameter name: #{arg_name}", arg_location
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would need to do something like:

if args.any? &.name.==(arg_name)
  raise "duplicated fun parameter name: #{arg_name}", arg_location
end

since args is an array of objects and not just a simple array of strings or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was copied from the check for defs which also needed to look at the external parameter names (any? would require two iterations as the error messages are different).

I don't think any? here is necessarily cleaner than the current approach.

@straight-shoota straight-shoota added this to the 1.5.0 milestone May 30, 2022
@straight-shoota straight-shoota merged commit 9b8870c into crystal-lang:master Jun 1, 2022
@HertzDevil HertzDevil deleted the bug/duplicate-fun-param branch June 1, 2022 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants