GDScript: Fix some issues with assignments that involve untyped things

[vonagam]

var hack = 0

func test():
  var number = 0
  number = 1 # currently type unsafe, should be safe
  var inferring := number # currently invalid, should be ok
  
  var any: Variant = 0
  any = '1' # currently type unsafe, should be safe
  
  self.hack = '?'
  var hacked := hack # currently type safe, should be invalid

As show with number variable a single assignment (even of the same type) currently makes a weakly inferred variable a variant.

I changed the logic about downgrading of such weakly types:

• Now both assignee and assigned can be downgraded, not only assignee as right now.
• Downgrade happens only when there is some mismatch (indication that variable maybe intended to contain other types), not on any assignment as right now.
• To get source for a type SubscriptNode now handled too, not only direct IdentifierNode, so it means that it is possible to downgrade member variables of other classes now and that self.prop will not be skipped.

The fact that it is possible to downgrade member variables of other classes means that theoretically decision on which type of assignment to use (conversion one or a basic) can be moved from analyzer to compiler in the future.

(Also added one line to set is_self to true in reduce_call if a call happens on a literal self, it is unrelated, can remove.)

[vonagam]

Rebased to resolve conflicts with #62688. Fixed its regression:

func test():
  var foo: bool = true
  foo += 'bar' # this should be invalid

[kleonc]

var hack = 0

func test():
  var number = 0
  number = 1 # (1) currently type unsafe, should be safe
  var inferring := number # (2) currently invalid, should be ok
  
  var any: Variant = 0
  any = '1' # (3) currently type unsafe, should be safe
  
  self.hack = '?'
  var hacked := hack # (4) currently type safe, should be invalid

I agree with (3), (4).
(1) Makes sense, number is untyped so any assignment is safe.
(2) I disagree, number is untyped so inferring shouldn't be allowed. I don't see how it's different from (4).

[vonagam]

About (2). This currently works - var number = 0; var inferring := number. I just avoided removing type from number on any assignment. I'm okay with forbidding explicit inference with not-explicitly typed variables, but everyone needs to agree to that.

About (1). "is untyped so any assignment is safe" - don't think it is true. It is safe for var number = 0; number = 1 because we know that number was assigned an int at initialization, so the same type should be type safe, but if you do var hm = 0; hm = 'maybe' then it is valid, but we cannot guarantee if it is type safe or not, so should be marked as type unsafe.

[vonagam]

Small change - added same downgrade logic to initializer too:

var x = 'weak'
# somewhere after that:
var y: int = x

We hope that by the time the assignment will happen x will be int, mark initializer as unsafe and ask for conversion check in runtime. But it also means that x is theoretically not only string, so now we downgrade it to weak variant.

[vonagam]

Rebased to resolve small conflict with #71192, which was fixing the issue (one of) already fixed here.

[vnen]

The only thing about inferred types is that if we allow something like:

var untyped = 5
var typed := untyped

then we have to be 100% sure that all inferred types are always correct. There has been cases in the past where the analyzer were assigning inferred types that weren't guaranteed to be true. The trivial case I've shown here is obvious but I assume this means any INFERRED datatype would be accepted, so this needs to be really solid.

It would naturally be a good thing if inferred types were always correct (and downgraded to UNDETECTED on any doubt), but it's not trivial to guarantee this in all cases. I'm not sure if all issues in this matter have been resolved. Since I'm not yet familiar with all the changes, if this was already worked on then please let me know.

[vonagam]

var untyped = 5
var typed := untyped

Currently works, yes. Actually forgot to ask about that at last meeting, was planning to do so tomorrow.

[vonagam]

But for record, this thing var typed := untyped is unrelated to this PR, this is about assignments, not initialization of assignables. I think it is correct to remove ability to infer on weakly typed input, but it should be done in other PR. I used this example just to demonstrate that weak variable does not become a bare variant on an assignment that matches its weak type, unfortunately there is no simple way to demonstrate that for tests.

[vnen]

Well, it can be done in different PR, but if tests rely on this then such PR would have to change this test as well (or remove them).

In this sense, I think it would be better to change the tests here to not rely on this, or remove them altogether (I get it's bad to not have tests, but have some relying on unintended behavior is also bad IMO).

[vnen]

Apart from my comments the PR is good and could be merged. If anything we sort the tests in another PR.

[akien-mga]

Thanks!