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

Make GDScript allow some keywords as identifiers #8217

Merged
merged 1 commit into from
Jul 25, 2017

Conversation

bojidar-bg
Copy link
Contributor

Fixes #8085

Here is a test script:

extends SceneTree

func log(a): # Call with self.log()
	print(a)

func _initialize():
	var dict = {
		"true": 1,
		"false": 1,
		"null": 1,
		"sync": 1,
		"remote": 1,
		"slave": 1,
		"if": 1,
		"match": 1,
	}

	print(dict.true)
	print(dict.false)
	print(dict.null)
	print(dict.sync)
	print(dict.remote)
	print(dict.slave)
	print(dict.if)
	print(dict.match)
	print(log(1))
	print(self.log(1))
	quit()

Here are some things that won't work with this changes:

var var = 1
var const = 1
var func = 1

func and(a, b): # Technical issues for this one, since it is equivalent to func &&(a, b):..

And here are some strange ones that would work

func var(): pass # Call with self.var()
func func(): pass # self.func() or just func() if lambda isn't merged
class class:
  static func static(): return true
func _ready():
  self.var()
  self.func()
  print(class.static()) # true

Copy link
Contributor

@karroffel karroffel left a comment

Choose a reason for hiding this comment

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

Looks good so far, I'd keep the old error messages though.


_set_error("Expected identifier for argument.");
_set_error("Expected literal for argument.");
Copy link
Contributor

Choose a reason for hiding this comment

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

literal? It's still an identifier. It's just that the identifier might have the same name as a keyword, it's an identifier nonetheless.

Usually "constant values" (like a string or a number) are called literals. I'd keep the old error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ookay, would revert that part.

if (tokenizer->get_token() != GDTokenizer::TK_IDENTIFIER) {
_set_error("Expected identifier for setter function after 'notify'.");
if (!tokenizer->is_token_literal()) {
_set_error("Expected literal for setter function after 'setget'.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Old relict? 😄 This is more descriptive, cool. But I'd still change it to back to identifier.

@bojidar-bg bojidar-bg force-pushed the gdscript-fix-keyword-call branch from 6382a28 to 7e08b44 Compare March 31, 2017 17:57
@bojidar-bg
Copy link
Contributor Author

@karroffel Fixed

Copy link
Contributor

@karroffel karroffel left a comment

Choose a reason for hiding this comment

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

👍 Looks good!

@akien-mga
Copy link
Member

Since we now prevent naming variables like properties of the base class (e.g. if you name a member variable position or rotation in a Node2D, the debugger will raise an error), I think it's unnecessary to allow some half-shadowing of keywords and global scope methods. Because to be consistent we'd have to allow the same for member variables, and it would just become very confusing.

I think Godot users will learn to live with this limitation, it might be frustrating a couple times, but then you make do and benefit from the consistency.

@bojidar-bg
Copy link
Contributor Author

@akien-mga Still, I think we should allow calling methods like sync or log, even if we disallow creating variables or methods with such names.

@akien-mga
Copy link
Member

Well if we disallow creating them, it makes no sense to allow calling them, since the situation would never arise.

@bojidar-bg
Copy link
Contributor Author

@akien-mga That's wrong, since we can just have a GDNative or C# script which would be free to register them...

@akien-mga
Copy link
Member

So what should we do here (apart from a needed rebase)?

@vnen
Copy link
Member

vnen commented Jul 11, 2017

I'm OK with this change as it fixes some current problems, like String.match() that can't be called. Creating functions with keywords as names should be documented to explain the need of the self. prefixing.

@akien-mga
Copy link
Member

@bojidar-bg You've got the go from @reduz, albeit you need to add some more documentation (and rebase):

<reduz> it's ok, but i would add a bit more doc on the source code to make sure what everything is doing and why, since it's clearly an usability hack
<reduz> as in, anyone who looks at the source eould expect to find TK_IDENTIFIER in the switch, for example
<reduz> so explaining those hacks would be a good idea

@bojidar-bg bojidar-bg force-pushed the gdscript-fix-keyword-call branch from 7e08b44 to e28d4d4 Compare July 23, 2017 13:12
@bojidar-bg
Copy link
Contributor Author

bojidar-bg commented Jul 23, 2017

Ok, rebased, now I would have to think how to include the doc. 😃 Edit: done

Fixes godotengine#8085
Added some comments around the use of is_token_literal, as discussed.
@bojidar-bg bojidar-bg force-pushed the gdscript-fix-keyword-call branch from e28d4d4 to 1936e1d Compare July 23, 2017 20:35
@reduz
Copy link
Member

reduz commented Jul 23, 2017 via email

@akien-mga akien-mga merged commit e00630b into godotengine:master Jul 25, 2017
@Geequlim
Copy link
Contributor

After this commit the GDScript is totally broken!

image

image

@akien-mga
Copy link
Member

Let's give @bojidar-bg some time to try a fix :)

@akien-mga
Copy link
Member

@Geequlim Fixed by #9846.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants