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

[typer] Delay typer creation to after init macros #11323

Merged
merged 18 commits into from Oct 18, 2023
Merged

Conversation

kLabz
Copy link
Contributor

@kLabz kLabz commented Oct 4, 2023

Following #11043, this PR turns the warnings (previously only enabled with -D haxe-next) into errors and actually delay the typer creation to after init macros.

This avoids ending up with more typing contexts than needed, which were leading to some nasty errors (including some infamous "type X is redefined from X" ones). This happened because init macros (or even other macros without the macro API separation) could change the context signature by altering defines.

A follow up will be needed, to ensure 4.3_bugfix displays warnings for all macro API that this PR covers (a few more than in previous PR) when using -D haxe-next.

I think this closes #7871 ?

@kLabz kLabz added this to the Release 5.0 milestone Oct 4, 2023
src/compiler/compiler.ml Outdated Show resolved Hide resolved
@Simn
Copy link
Member

Simn commented Oct 4, 2023

I think we should factor out some of the "harmless" changes here, like the stuff that gets moved around and such. That way we can merge that immediately and then look into the actual delay change, which I expect to break a few things.

@kLabz
Copy link
Contributor Author

kLabz commented Oct 4, 2023

I'm not sure much would break; I expect much more issues with the macro API restrictions, but that's expected and will have to be fixed on user/libs side for Haxe 5 anyway.

I will still separate in two PRs, if only to make review easier.


Hmm, moving com.global_metadata around without the typer delaying will apply @:build added by addGlobalMetadata to types in macro context too.. unfortunate 😖

Guess I'll have to remove that change from the preparation branch then.

@kLabz kLabz changed the base branch from development to delay_typer_preparation October 4, 2023 10:03
@kLabz
Copy link
Contributor Author

kLabz commented Oct 4, 2023

Rebased delay_typer_preparation (#11324) and set it as base for this PR to reduce the diff.
Will set the base back to development when #11324 is merged.

@kLabz kLabz changed the base branch from delay_typer_preparation to development October 4, 2023 11:09
@skial skial mentioned this pull request Oct 4, 2023
1 task
@kLabz
Copy link
Contributor Author

kLabz commented Oct 5, 2023

@SomeRanDev could you try this branch with some reflaxe target to check if I didn't break it?

There are builds available on build.haxe.org:

@SomeRanDev
Copy link
Contributor

No problem! Thanks for the build links! 👍

Unfortunately, I have encountered an issue. As far as I can tell, it's NOT from this branch, rather it was introduced from the custom target pr a couple months back (happens with every development build since then).

I'm a little embarrassed I didn't notice this until now, here's how it happens:


Minimal example project: unsupported_error.zip

Uncaught exception unsupported
[CALLSTACK] : Called from here

If the early __.Init.init() call is made, it causes some Compiler functions to trigger the above error. This happens even after the init macro phase.

I'm sure there's more, but the two I found were:

  • haxe.macro.Compiler.addMetadata
  • haxe.macro.ClassField.expr() will cause this even if the ClassField is discovered in the "Context.onAfterTyping" callback

Note the Init.init() function can be completely blank and this issue still occurs. If the Init module is renamed or deleted, everything works again.

It seems like the incomplete macro api is making it past the init macro phase?

@kLabz
Copy link
Contributor Author

kLabz commented Oct 5, 2023

Thanks!
Indeed custom target init macro context was not cached (and thus not updated later with enhanced macro API). Provided example project now works.

New build:

@Simn
Copy link
Member

Simn commented Oct 5, 2023

Although it's unlikely to break again, it would be good to add a test for that exact situation.

@kLabz
Copy link
Contributor Author

kLabz commented Oct 5, 2023

Yeah, was planning to do that; along with (testing) other things that could be broken with custom targets.

@SomeRanDev
Copy link
Contributor

Sorry for the delay! Tried a bunch of different reflaxe and platform configs and nothing seemed to break or look weird, so looking good for me.👌

# Conflicts:
#	src/compiler/compiler.ml
@Simn Simn merged commit 5a3b9d5 into development Oct 18, 2023
122 checks passed
0b1kn00b pushed a commit to 0b1kn00b/haxe that referenced this pull request Jan 25, 2024
* Move global_metadata to common context

* Revert "Move global_metadata to common context"

This reverts commit 319e074.

* [typer] delay typer init to after init macros

* [debug] add compiler stage debug

* [typer] fix macro context when no init macros

* [typer] promote macro api to full api after init macros

* [typer] remove CTyperCreated stage

* [display] restore DisplayProcessing

* [typer] rework after init macro

* [typer] fix macro api context when no init macros

* [macro] don't reset static prototype after init macros

* small cleanup

* Remove MacroLight, simplify init macro context creation

* Reduce diff

* Cleanup interp cache handling

* [custom targets] enable macro cache before setting up targets

* [tests] add test for custom target and macro com API limitations

---------

Co-authored-by: Simon Krajewski <simon@haxe.org>
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.

[macro] Detect some invalid usages of the haxe.macro.Compiler API
3 participants