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

GDScript: writing only "array[i]" doesn't trigger an error #7302

Closed
Zylann opened this issue Dec 14, 2016 · 16 comments
Closed

GDScript: writing only "array[i]" doesn't trigger an error #7302

Zylann opened this issue Dec 14, 2016 · 16 comments

Comments

@Zylann
Copy link
Contributor

Zylann commented Dec 14, 2016

Tested on my Godot fork (last update 4d8bed3)
Windows 10 64 bits

I wrote this in GDScript:

	for i in range(0, index):
		pts[j]
		j += 1

I forgot to assign a value to pts[j] or do anything with it. However, it didn't trigger any error, the code probably did nothing at all. It took me some time to find this, it would be nice for Godot to indicate this as an error.

@bojidar-bg
Copy link
Contributor

bojidar-bg commented Dec 15, 2016

I'd vote for closing this as a non issue. Here is an example:

class Test:
  var t = 0 setget test_set, test_get
  func test_set(value):
    t = 0
  func test_get():
    t += 1
    return t

func test():
  return Test.new()
func something():
  test() # Valid.
  1 # Invalid??
  var _test = Test.new() # Valid.
  _test.t = 5 # Valid.
  _test.t # Invalid?? (-> getter modifies value..)
  test().t # Invalid?? (-> getter modifies value, even if it is dropped right afterwards)
  """ Multiline "comment", which is actually a string
  line 2""" # Invalid??
  var comment = """A multiline remark that goes
  in a variable
  even if it looks ugly""" # Valid.

Most times, you don't want the editor shouting at you, just because you didn't assign a value of some expression to something. The reason is that sometimes, getting the value has "useful" side-effects, or you actually mean to drop it.
If we are going to push this on, shouldn't we enforce saving the value of every function call's return in a variable? That'd be terrible.
Also, not many mainstream language have implemented this (I don't think I know of any example). JavaScript, (CoffeScript), Python, C++, C#, (PHP), and many other imperative object-oriented programming languages from which we have taken much inspiration don't have it at all. Well, C++ has a warning about it, but still.
(/ 5¢)

@akien-mga
Copy link
Member

The test of fire:

Python 3.5.2 (default, Sep  7 2016, 09:19:15) 
[GCC 5.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> pts = [0, 1, 2, 3]
>>> j = 2
>>> pts[j]
2

pts[j] is a valid statement, it just returns a value which will do nothing in its scope, but it's treated the same way as other kind of statements that return a value (e.g. calling a function that returns an error code without putting it in a variable).

If we had a mechanism for warnings though, that's the kind of thing that might be worth mentioning (just like a C/C++ compiler can warn you about unused return values), but it should not be an error.

@Zylann
Copy link
Contributor Author

Zylann commented Dec 15, 2016

@akien-mga Python was written to also work in interactive mode so it makes sense here, however the way GDScript is used (in files, where you don't care about it) makes this a bit odd (and nothing uses that value)
Also I feel the getter test isn't relevant because writing just the get still looks wrong in the first place (I'd even say the race condition in it is bad practice, I never ever had to write such a thing or seen it).

Despite all this, if you still feel it's a non-issue I would indeed like a warning system obvious enough to catch this (like line highlight).

Note: C# doesn't allow me to write this.
Note 2: Java doesn't allow to write this either.
Note 3: D also gives an error.

Most script languages support it either because of permissive nature or the fact they actually need to (interactive mode, or the fact it used to be a template language for PHP).

@akien-mga
Copy link
Member

Python was written to also work in interactive mode so it makes sense here, however the way GDScript is used (in files, where you don't care about it) makes this a bit odd (and nothing uses that value)

Try the same in C++ ;)

@Zylann
Copy link
Contributor Author

Zylann commented Dec 15, 2016

I know some other languages have this, but GDScript is a new language tailored for a specific use, right? My mistake was really stupid and won't probably do it again (but I'm human, can't say for sure^^). Still, it would be really nice to have at least a warning.

@bojidar-bg
Copy link
Contributor

Again, if we are doing this shouldn't we also highlight lines such as:

func _ready():
  get_pos() # <-??

@Zylann
Copy link
Contributor Author

Zylann commented Dec 15, 2016

This is syntactically a function call, so obviously it's not possible to issue such a warning on it.

@RebelliousX
Copy link
Contributor

RebelliousX commented Dec 15, 2016

Python was written to also work in interactive mode so it makes sense here, however the way GDScript is used (in files, where you don't care about it)

Not really, for example, C++ is not written to be used in interactive mode and still allow such expressions without warnings or errors. It just ultimately evaluates to true or false.

@Zylann
Copy link
Contributor Author

Zylann commented Dec 16, 2016

Well, that's how I interpret it in terms of usage. In Python or PHP it shows a good one. What is the actual usage of allowing that in GDScript? (Same question for c++) Or is it just a side effect of the implementation? I'm nitpicking but so far I see none, in the context of what the language was made for.

@RebelliousX
Copy link
Contributor

RebelliousX commented Dec 17, 2016

@Zylann Speaking about C++, such expressions can be used to quickly evaluate any object to be boolean. It goes way back in C days, when prorammers test if a non-boolean variable is true by doing simply if(a) b; And they can enforce boolean state and quickly test any object for being true by using double bang operator !!.

Also, the semicolon ; is a valid operator by itself, that if used alone in a line, it will be converted to a `NOP' instruction in machine code, this can be used fo example to test how fast an empty loop iterates.

I think it is a key feature not a bug in the language. Although I agree that it should give a warning about such use, that is, if there is code analysis functionality available or debugger warnings can be disabled. Otherwise halting execution for this is a bit too much.

And using the example bojidar-bg provided, what if a function that does one thing and returns some value and you decided not to do anything with it, are you supposed to create a temp var just to be discarded afterwards! This could have performance implications by creating and destroying objects, even if we are mostly using references.

@Zylann
Copy link
Contributor Author

Zylann commented Dec 17, 2016

@RebelliousX I was primarily asking about GDScript, and what usage this could have (C++ inherited a lot of features over time). GDScript is higher-level and has no semicolons. I also agree that function calls cannot be checked since they explicitely execute "logic" that can't be assumed to be a get operation. There are properties of course that can trigger a function, but as I said earlier, relying on that to write an unused get is against properties design and bad practice (IMO).

A warning would be fine :)

@RebelliousX
Copy link
Contributor

RebelliousX commented Dec 17, 2016

@Zylann You missed my point, what I meant about the example of the function call that returns a value applies to expressions that don't do anything. In fact, both will look the same in the end when converted in bytecode, the function call is simply a different execution block or stack frame, the original stack frame sees nothing but the returned value, not the logic of the function.

I mentioned C++ since you said that your question applies to C++ too. I just gave you an example of an expression which is the empty statement the semicolon.

Well this issue doesn't really bother me, I have seen worse that the compiler errors were suppressed when they shouldnt. Leading to hard to debug code because it goes unnoticed. But I dont recall the issue, it escaped my mind.

@Zylann
Copy link
Contributor Author

Zylann commented Dec 17, 2016

I didn't say it applied to C++ (well C++ allows this but other languages don't). An editor warning actually makes sense because if the compiled bytecode doesn't tell the difference, a parser can. It would then fall into these "good practice warnings" that don't prevent the code to run, but are worth mentionning.

@ghost
Copy link

ghost commented Apr 10, 2018

Still reproducible/relevant in 3.1 master 48890b1 : it doesn't trigger error.

@vnen
Copy link
Member

vnen commented Apr 12, 2018

GDScript would benefit in a system for warnings where this kind of things could be reported.

@vnen
Copy link
Member

vnen commented Sep 4, 2018

Closing since this is now a warning.

@vnen vnen closed this as completed Sep 4, 2018
@vnen vnen added this to the 3.1 milestone Sep 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants