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: Add unsafe_assignment warning #83697

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ryanabx
Copy link
Contributor

@ryanabx ryanabx commented Oct 20, 2023

This PR adds a new unsafe_assignment warning, which optionally can be enabled to warn the user if they are implicitly assigning a value of unknown type to a variable without casting it.

NOTE: This warning conflicts with the existing unsafe_cast warning, and this PR should include discussion on whether we prefer warning for unsafe casting or for unsafe assignment.

Examples of both warnings:

var dict: Dictionary = {
    "value": "foo"
}

var unsafe_cast: String = dict.value as String # unsafe_cast warning
var unsafe_assignment: String = dict.value # unsafe_assignment warning

CC @dalexeev since we were talking about a separate discussion of pattern matching and the topic of unsafe casting was brought up and whether unsafe assignment should be a warning.

@ryanabx ryanabx requested review from a team as code owners October 20, 2023 20:20
@ryanabx ryanabx changed the title Add unsafe_assignment warning GDScript: Add unsafe_assignment warning Oct 20, 2023
@ryanabx
Copy link
Contributor Author

ryanabx commented Oct 20, 2023

Currently, the CI tests fail because of the GDScript test suite. If the PR gets accepted, we can modify the GDScript tests to accomodate this warning.

@dalexeev
Copy link
Member

In fact, as I wrote in Rocket Chat, I have an outdated branch with a more comprehensive solution. This affects not only resolve_assignable(), but also reduce_assignment(), and the fact that assignment can be combined with another operation (for example +=). Also, this builds on #76020 to avoid making many changes to the tests and allow this warning to be ignored en masse. The main issue I've encountered and put off this PR is the need to refactor DataType to solve the problem with weak inferred types on hard Variants.

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.

2 participants