Skip to content

Re-enable checks for final access.#415

Open
EliteMasterEric wants to merge 12 commits into
experimentalfrom
final-access
Open

Re-enable checks for final access.#415
EliteMasterEric wants to merge 12 commits into
experimentalfrom
final-access

Conversation

@EliteMasterEric

Copy link
Copy Markdown
Member
  • Improve error messages for invalid access to final variables in scripts.
  • Improve handling of properties with null or never setters (Haxe never allows assignment to never, and only allows local assignment to null).
  • Re-enable the code to prevent assignment to final constants from Haxe code. This prevents issues like being able to re-assign values despite them being defined as immutable constants.

@EliteMasterEric

Copy link
Copy Markdown
Member Author

Oh yeah if you're wondering why it's a draft right now it's because it doesn't fucking work

@EliteMasterEric EliteMasterEric marked this pull request as ready for review June 13, 2026 07:19
@EliteMasterEric

EliteMasterEric commented Jun 13, 2026

Copy link
Copy Markdown
Member Author
image

It works now!

I mainly wrote this to help clean up FunkinCrew/Funkin#7238, so we can rewrite the variables to recycle them without the scripts being able to edit them from the outside.

Definitely should get some review from contributors but this should prevent people from accidentally messing with constants when they aren't supposed to.

An aside, this should be good to merge as-is, but in a future PR it might be nice to add functional metadata that, when applied to a class/method/block, skips this check (in a similar manner to @:privateAccess in Haxe source).

@NotHyper-474

NotHyper-474 commented Jun 14, 2026

Copy link
Copy Markdown
Collaborator

On the topic of contributors reviewing, I've noticed the GCC C++ compiler would baloon in memory usage then die with this PR due to the generated meta code being too massive (and this, infact, would not be the first time this happens with this sort of macro), which prevented me from testing the code on Linux.

We likely need to rework the macro to use a Resource, akin to what #366 and some other PRs before it did.

@EliteMasterEric

Copy link
Copy Markdown
Member Author

We likely need to rework the macro to use a Resource, akin to what #366 and some other PRs before it did.

I had the same issue, which is why it already uses a Resource (that probably changed between when you reviewed it and when I marked the PR as not draft).

Please review it again if you can.

@NotHyper-474

Copy link
Copy Markdown
Collaborator

Oh, I can't believe I've missed that lol. I can confirm it compiles with no issues on my end now (besides me having to locally fix all the null-safety errors that came with the nullable scriptInit commit).

The only thing I found odd was that the changes from #416 were kind of reverted, was that intentional?

@TechnikTil TechnikTil left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good to me!

@NotHyper-474 NotHyper-474 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One issue I've been running with this PR are these invalid access errors that seem to specifically happen with static field access (or in this case GameOverSubState.musicSuffix)

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants