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

Returns null and does not cache when the source code of the script fails to load #76954

Merged

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented May 11, 2023

This usually means that an ERR_FILE* error occurred.

Fix #76941.

Previously, using GDScriptCache::get_full_script() would ignore errors during loading. Now, all errors are not ignored.

Judging in which period the error occurred, it can be judged based on the return value:

  1. null + err: Error during script loading (load_source_code()).
  2. script + err: Error during script parsing.

@Rindbee Rindbee requested a review from a team as a code owner May 11, 2023 14:54
@akien-mga akien-mga requested a review from adamscott May 12, 2023 08:57
@akien-mga akien-mga added this to the 4.1 milestone May 12, 2023
@anvilfolk
Copy link
Contributor

anvilfolk commented Jun 15, 2023

I'm not going to review because I am not confident of my knowledge of a lot of this meta-gdscript stuff, but I am a little wary of this approach!

My understanding is that GDScript tries pretty hard to not fail-fast, I think. If there's an error in parsing, we try to continue and parse as much as possible. I think that might be part of the reason why ResourceFormatLoaderGDScript::load() instantiates the script. Similarly, I think that's why we didn't return an empty script from get_shallow_script(). This might allow other scripts to attempt to analyze/compile despite other files having errors.

So I am particularly worried about changes in the user experience when people are writing code and causing certain errors as they edit. I wouldn't want half the project to halt/give errors because a single file currently has a parsing error.

But this is just my very limited understanding, so I would love to hear more from @vnen and @adamscott!

That said, I do think if there's an error, it ought to be reported by ResourceFormatLoaderGDScript::load(), as opposed to pretending there's no error by returning OK. That way the code can figure out what to do with that downstream, whether to handle it gracefully or throw an error itself, etc :)

@Rindbee
Copy link
Contributor Author

Rindbee commented Jun 15, 2023

Previously, errors during loading were ignored. Now, all errors are not ignored.

Judging in which period the error occurred, it can be judged based on the return value:

  1. null + err: Error during script loading (load_source_code()).
  2. script + err: Error during script parsing.

…ils to load

This usually means that an `ERR_FILE*` error occurred.

Previously, using `GDScriptCache::get_full_script()` would ignore errors during loading.
Now, all errors are not ignored.

Judging in which period the error occurred, it can be judged based on the return value:
1. null + err : Error during script loading (load_source_code()).
2. script + err: Error during script parsing.
@Rindbee Rindbee force-pushed the return-null-on-fail-load-script branch from 33405de to cbce374 Compare June 15, 2023 13:11
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Seems to do the job! Thanks @Rindbee!

@akien-mga akien-mga merged commit 5f9175f into godotengine:master Jun 19, 2023
@akien-mga
Copy link
Member

Thanks!

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.

[GDScript] Loading non-existing gdscript returns valid instance.
4 participants