Check that def, macro, and block parameters don't end with ? or !

[potomak]

Fixes #10917

Tested with:

$ crystal spec/compiler/parser/parser_spec.cr
[...]
Finished in 212.78 milliseconds
2656 examples, 0 failures, 0 errors, 0 pending

[potomak]

Note: this is a breaking change.

I decided to raise an exception because I didn't find how to log a warning + test warning message. In parser_spec.cr I just found it_parses and assert_syntax_error.

What do you think of this approach?

In case you suggest to keep this change backward compatible can you point me to an example of warning + test for the parser please?

[straight-shoota]

This should not be a breaking change. So we'll need a warning.

You can use @program.report_warning or .report_warning_at for that

crystal/src/compiler/crystal/semantic/warnings.cr

Lines 19 to 39 in e10841c

	def report_warning(node : ASTNode, message : String)
	return unless self.warnings.all?
	return if self.ignore_warning_due_to_location?(node.location)
	self.warning_failures << node.warning(message)
	end
	def report_warning_at(location : Location?, message : String)
	return unless self.warnings.all?
	return if self.ignore_warning_due_to_location?(location)
	if location
	message = String.build do |io|
	exception = SyntaxException.new message, location.line_number, location.column_number, location.filename
	exception.warning = true
	exception.append_to_s(io, nil)
	end
	end
	self.warning_failures << message
	end

There are currently no warnings in the parser and no specs for that.
You should be able to use the assert_warning spec helper. Or adapt it to a variant that does only syntax parsing and skip the semantic stage. But feel free to leave that out.

[HertzDevil]

As I mentioned somewhere else (in the Int128 literals issue I think), the parser does not have access to the Crystal::Program because it doesn't need one, so the warnings have to be collected through some other means.

[straight-shoota]

Ah, okay. Then we'll probably need a bigger change in order to move forward with this.
If you want to investigate this, @potomak, please go ahead. But don't hesitate to drop this for now.

[potomak]

@straight-shoota would it make sense if I add @program to Parser and then use report_warning instead of raising and exception?

I guess it would be better to split the work in two PRs, one for updating the definition of Parser and an update of this one on top of that.

Any objections?

[HertzDevil]

I think it should be the opposite: the parser itself should be able to report warnings even without a Crystal::Program.

[straight-shoota]

Yeah, I suppose it would be preferable to collect warnings for an individual parser run vs. amending everything in Program. The reason for that is that there are some locations where individual parser instances are used during the compiler stages (for example for (re-)parsing macro code or in the interpreter REPL). It would be good if we could easily identify warnings in a particular parser run instead of having all merged together in the Program's warnings.

[HertzDevil]

@potomak are you interested in turning those checks into syntax warnings similar to #12427?

[potomak]

@potomak are you interested in turning those checks into syntax warnings similar to #12427?

@HertzDevil yes and sorry for the late reply.

[Sija]

Small optimization (no regex) + rephrased error message to make it more clear about the name in question:

Suggested change

	if param_name.ends_with?(/\?|!/)
	raise "invalid param name"
	end
	if param_name[-1]?.in?('?', '!')
	raise "invalid parameter name: #{param_name}"
	end

[potomak]

When I use this version I get a bunch of Index out of bounds (IndexError) errors.

I guess I should first check if the param_name string is not empty, what do you think?

[straight-shoota]

You can go with this:

Suggested change

	if param_name.ends_with?(/\?|!/)
	raise "invalid param name"
	end
	if param_name.ends_with?("?") || param_name.ends_with?("!")
	raise "invalid param name"
	end

[Sija]

I've edited the suggestion

[potomak]

Ok, so the choice is between:

param_name[-1]?.in?('?', '!')

!param_name.empty? && param_name[-1].in?('?', '!')

param_name.ends_with?("?") || param_name.ends_with?("!")

I think the last one is the most clear.

Is there an advantage in using version 1 or 2?
Which one is the most idiomatic?

[potomak]

When external_name is present external_name_token should also be present, so it may be better to turn this into an invariant.

[oprypin]

I've brought this change up to date. Actually it looks good, it emits warnings, as was discussed. Not sure why this one got left behind.

[nobodywasishere]

There's one file in the stdlib that will give a warning for this, don't remember which file though. May want to fix that at the same time

[oprypin]

@nobodywasishere That is true. But that instance cannot be fixed without fixing REPLy first - it defines a base class with a parameter that contains a questionmark. So sent I3oris/reply#9

[Sija]

I wish the decision to forbid such suffixes was revisited... Given that they're allowed in the method names I see no logical reason to do so.

[nobodywasishere]

@Sija the current syntax allows this, which I personally think is problematic:

var = 5
var! : Int32 = 2
# parsed as `(var) != (1)`
puts var!=1   # => true
# parsed as `(var!) = (1)`
puts var! = 1 # => 1

[keidax]

Given that they're allowed in the method names I see no logical reason to do so.

I think that's exactly the reason to make this change -- so there is no ambiguity about whether color? is a method or a variable.

[ysbaddaden]

@keidax a simple color name is just as ambiguous: is it a method or a local variable?