GDScript: Improve call analysis #75988

dalexeev · 2023-04-12T14:43:04Z

Closes Object.new() can't be used (but it works on any Object-derived class like Node or RefCounted) #73213.
Closes Trying to invoke un-existing static methods does not yield a parser error #73283.
Add missing UNSAFE_CALL_ARGUMENT warning.

modules/gdscript/gdscript_warning.cpp

adamscott · 2023-04-24T12:50:14Z

Notes from the GDScript meeting:

The missing warning is a problem;
The error message should be changed. Something like: "the function expects an int, but the value is only inferred" instead of "int (variant)";
The formatting of DataType to string should maybe not change, or at least, this should belong in another PR.

dalexeev · 2023-04-25T06:52:51Z

I removed the DataType::to_string() changes from this PR. In the future, we may investigate this issue separately.

modules/gdscript/gdscript_analyzer.cpp

anvilfolk

A small typo noticed, and needs a rebase, but is there a reason this hasn't been merged? It feels like it's good to go otherwise :)

modules/gdscript/gdscript_warning.cpp

modules/gdscript/tests/scripts/analyzer/warnings/unsafe_call_argument.gd

anvilfolk

I just noticed some discrepancies in whether mark_node_unsafe should be protected by #ifdef DEBUG_ENABLED compared to existing code. I didn't do a thorough review of the analyzer, but I believe there's an existing pattern that isn't being followed here that could have unintended consequences?

anvilfolk · 2023-05-05T13:54:14Z

modules/gdscript/gdscript_analyzer.cpp

@@ -4838,18 +4838,33 @@ void GDScriptAnalyzer::validate_call_arg(const List<GDScriptParser::DataType> &p

 		if ((arg_type.is_variant() || !arg_type.is_hard_type()) && !(par_type.is_hard_type() && par_type.is_variant())) {
 			// Argument can be anything, so this is unsafe.
+#ifdef DEBUG_ENABLED
 			mark_node_unsafe(p_call->arguments[i]);


So all of the push_warning() that I saw in the analyzer were protected by DEBUG_ENABLED, but none of the mark_node_unsafe() calls were. I think it might make sense to pull this one outside the #ifdef block here?

godot/modules/gdscript/gdscript_analyzer.cpp

Lines 5117 to 5127 in f6bf51c

void GDScriptAnalyzer::mark_node_unsafe(const GDScriptParser::Node *p_node) {

#ifdef DEBUG_ENABLED

if (p_node == nullptr) {

return;

}

for (int i = p_node->start_line; i <= p_node->end_line; i++) {

parser->unsafe_lines.insert(i);

}

#endif

}

hahahahah, well, that resolves it as not mattering for sure :)

modules/gdscript/gdscript_analyzer.cpp

anvilfolk

LGTM :)

dalexeev · 2023-06-08T16:48:28Z

I extended this PR. Please remove the cherrypick:4.0 label.

santouits · 2023-06-08T20:52:46Z

Should you remove this code too?

godot/modules/gdscript/gdscript_analyzer.cpp

Lines 634 to 638 in e188d61

    
           } else if (first == SNAME("Object")) { 
        
           	// Object is treated like a native type, not a built-in. 
        
           	result.kind = GDScriptParser::DataType::NATIVE; 
        
           	result.builtin_type = Variant::OBJECT; 
        
           	result.native_type = SNAME("Object");

modules/gdscript/gdscript_analyzer.cpp

adamscott · 2023-06-22T15:36:54Z

Moved the milestone to 4.2, as we entered yesterday the release freeze for 4.1, where only urgent bugfixes are merged. Added the "cherrypick 4.1" tag as this would be interesting to patch 4.1.x with your PR.

adamscott · 2023-09-19T13:50:32Z

The GDScript team thinks that this PR should be merged for 4.2. We should take a look again at this before the feature freeze.

* Add missing `UNSAFE_CALL_ARGUMENT` warning. * Fix `Object` constructor. * Display an error for non-existent static methods.

vnen

Overall looks good to me.

YuriSizov · 2023-09-27T16:36:49Z

modules/gdscript/tests/scripts/runtime/features/stringify.gd

@@ -24,7 +24,8 @@ func test():
 	print(StringName("hello"))
 	print(NodePath("hello/world"))
 	var node := Node.new()
-	print(RID(node))
+	@warning_ignore("unsafe_call_argument")
+	print(RID(node)) # TODO: Why is the constructor (or implicit cast) not documented?


What do you mean with this comment? https://docs.godotengine.org/en/stable/classes/class_rid.html#class-rid

There is no RID(Object) constructor (or something like this).

Ah right, we should document the explicit conversion.

YuriSizov · 2023-09-27T19:01:32Z

Thanks!

dalexeev requested a review from a team as a code owner April 12, 2023 14:43

Calinou added bug topic:gdscript cherrypick:4.0 labels Apr 12, 2023

Calinou added this to the 4.1 milestone Apr 12, 2023

anvilfolk reviewed Apr 24, 2023

View reviewed changes

modules/gdscript/gdscript_warning.cpp Outdated Show resolved Hide resolved

dalexeev force-pushed the gds-unsafe-call-argument branch from daf2e20 to af6d89f Compare April 25, 2023 06:47

vnen reviewed Apr 26, 2023

View reviewed changes

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved

dalexeev force-pushed the gds-unsafe-call-argument branch 2 times, most recently from 06cfa39 to ce9c682 Compare April 27, 2023 06:54

anvilfolk suggested changes May 2, 2023

View reviewed changes

modules/gdscript/gdscript_warning.cpp Outdated Show resolved Hide resolved

modules/gdscript/tests/scripts/analyzer/warnings/unsafe_call_argument.gd Show resolved Hide resolved

dalexeev force-pushed the gds-unsafe-call-argument branch from ce9c682 to 8227ea1 Compare May 5, 2023 08:26

anvilfolk suggested changes May 5, 2023

View reviewed changes

anvilfolk approved these changes May 5, 2023

View reviewed changes

dalexeev force-pushed the gds-unsafe-call-argument branch from 8227ea1 to ac0db30 Compare June 8, 2023 16:44

dalexeev changed the title ~~GDScript: Add missing UNSAFE_CALL_ARGUMENT warning~~ GDScript: Improve call analysis (part 1) Jun 8, 2023

dalexeev requested a review from vnen June 8, 2023 16:48

dalexeev force-pushed the gds-unsafe-call-argument branch from ac0db30 to 10373d6 Compare June 8, 2023 17:08

akien-mga removed the cherrypick:4.0 label Jun 8, 2023

akien-mga requested a review from a team June 19, 2023 21:05

adamscott reviewed Jun 21, 2023

View reviewed changes

modules/gdscript/gdscript_analyzer.cpp Show resolved Hide resolved

adamscott modified the milestones: 4.1, 4.2 Jun 22, 2023

adamscott added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jun 22, 2023

dalexeev force-pushed the gds-unsafe-call-argument branch from 10373d6 to e1e0908 Compare July 2, 2023 13:08

dalexeev mentioned this pull request Aug 27, 2023

Object is many times heavier than Node or RefCounted...? #81014

Closed

akien-mga requested a review from a team August 27, 2023 21:18

dalexeev force-pushed the gds-unsafe-call-argument branch from e1e0908 to 1cf0775 Compare August 28, 2023 06:38

dalexeev removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023

GDScript: Improve call analysis

e8696f9

* Add missing `UNSAFE_CALL_ARGUMENT` warning. * Fix `Object` constructor. * Display an error for non-existent static methods.

dalexeev force-pushed the gds-unsafe-call-argument branch from 1cf0775 to e8696f9 Compare September 21, 2023 10:36

dalexeev changed the title ~~GDScript: Improve call analysis (part 1)~~ GDScript: Improve call analysis Sep 21, 2023

vnen approved these changes Sep 27, 2023

View reviewed changes

YuriSizov reviewed Sep 27, 2023

View reviewed changes

YuriSizov merged commit aa474c9 into godotengine:master Sep 27, 2023
15 checks passed

dalexeev deleted the gds-unsafe-call-argument branch September 27, 2023 19:05

Pennycook mentioned this pull request Jan 2, 2024

Compile Error Upon Install Pennycook/godot-utility-ai#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GDScript: Improve call analysis #75988

GDScript: Improve call analysis #75988

dalexeev commented Apr 12, 2023 •

edited

Loading

adamscott commented Apr 24, 2023

dalexeev commented Apr 25, 2023

anvilfolk left a comment

anvilfolk left a comment

anvilfolk May 5, 2023

dalexeev May 5, 2023

anvilfolk May 5, 2023

anvilfolk left a comment

dalexeev commented Jun 8, 2023

santouits commented Jun 8, 2023

adamscott commented Jun 22, 2023

adamscott commented Sep 19, 2023

vnen left a comment

YuriSizov Sep 27, 2023

dalexeev Sep 27, 2023

YuriSizov Sep 27, 2023

YuriSizov commented Sep 27, 2023

	void GDScriptAnalyzer::mark_node_unsafe(const GDScriptParser::Node *p_node) {
	#ifdef DEBUG_ENABLED
	if (p_node == nullptr) {
	return;
	}

	for (int i = p_node->start_line; i <= p_node->end_line; i++) {
	parser->unsafe_lines.insert(i);
	}
	#endif
	}

GDScript: Improve call analysis #75988

GDScript: Improve call analysis #75988

Conversation

dalexeev commented Apr 12, 2023 • edited Loading

adamscott commented Apr 24, 2023

dalexeev commented Apr 25, 2023

anvilfolk left a comment

Choose a reason for hiding this comment

anvilfolk left a comment

Choose a reason for hiding this comment

anvilfolk May 5, 2023

Choose a reason for hiding this comment

dalexeev May 5, 2023

Choose a reason for hiding this comment

anvilfolk May 5, 2023

Choose a reason for hiding this comment

anvilfolk left a comment

Choose a reason for hiding this comment

dalexeev commented Jun 8, 2023

santouits commented Jun 8, 2023

adamscott commented Jun 22, 2023

adamscott commented Sep 19, 2023

vnen left a comment

Choose a reason for hiding this comment

YuriSizov Sep 27, 2023

Choose a reason for hiding this comment

dalexeev Sep 27, 2023

Choose a reason for hiding this comment

YuriSizov Sep 27, 2023

Choose a reason for hiding this comment

YuriSizov commented Sep 27, 2023

dalexeev commented Apr 12, 2023 •

edited

Loading