fix: avoid compile dependencies for runtime-only constraint modules - #2913
fix: avoid compile dependencies for runtime-only constraint modules#2913jechol wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3598ed2fcd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3598ed2 to
7cd6bf8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cd6bf8b28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7cd6bf8 to
a8bb8d2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8bb8d2599
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # work and invoke a callback that is not required to tolerate a second invocation. | ||
| defp register({type, constraints}, env) do | ||
| type | ||
| |> Ash.Type.constraint_referenced_types(constraints) |
There was a problem hiding this comment.
I think we're going to have to make this opt-in, because there could be external types that have not added referenced_types but were relying on these compile time dependencies existing. We could turn it on in 4.0 perhaps and require that any type that references other types declare it in order to get compile time dependencies on their constraints.
There was a problem hiding this comment.
Switched it to opt-in:
config :ash, constraint_dependencies_from_referenced_types?: trueIt defaults to false. While off, constraints creates the compile dependencies it does today, so a type that does not implement referenced_types/1 loses nothing.
Ash's own config turns it on, the way bulk_actions_default_to_errors? does — otherwise the new path never runs in the test suite. I left it out of the installer: a dependency's type may not implement the callback, so it seemed better for it to arrive when the default changes in 4.0.
One thing I would like your read on.
set_type_transformation calls detect_type_cycle! with the constraints as written, but stores the ones init/1 returns. The persister reads what was stored, so a type whose init/1 consumes and drops a constraint it reads in referenced_types/1 is invisible to it. That is the difference from Spark reading the aliases in the DSL itself.
None of the built-in types remove such a constraint — they normalise it in place — so nothing is affected today. For now I documented it on c:referenced_types/1: it is called both before and after init/1, so it has to report the same types either way.
Closing it properly means carrying the pre-init walk through to the persister, which adds a field to seven entity structs, Ash.Resource.Attribute among them. It would also collapse the two type-graph walks into one.
I am unsure whether that is worth the surgery. In a private application, cutting 40 of 52 instance_of usages took a representative resource edit from 58 recompiled files to 41, so the accuracy of this mechanism matters to me — but if the documented contract is enough, I am happy to leave it there.
a8bb8d2 to
89df2bc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89df2bce5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
89df2bc to
90c143f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90c143fc62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # work and invoke a callback that is not required to tolerate a second invocation. | ||
| defp register({type, constraints}, env) do | ||
| type | ||
| |> Ash.Type.constraint_referenced_types(constraints) |
There was a problem hiding this comment.
When a custom Ash.Type.init/1 consumes a compile-only constraint such as :inner_type and returns normalized constraints without that key, this invokes referenced_types/1 on the post-init constraints and loses the reference. The same callback can correctly report the type during the pre-init cycle check, but once this option suppresses Spark's original constraint dependency, changing the nested type will no longer recompile the resource and its initialized state can remain stale. Capture the referenced types before initialization or carry that result through the entity transformation.
Useful? React with 👍 / 👎.
9472852 to
a89ac1f
Compare
A `constraints` value can hold module references with different dependency requirements. `instance_of` is only read at runtime, while a nested `type` is initialized while the DSL is compiled. Since `constraints` was unclassified, both created compile dependencies. Which of the two a reference is, is known by the type rather than the DSL, so `constraints` goes in `no_depend_modules` and the dependencies are registered from what `referenced_types/1` already reports, using the `Macro.Env` Spark persists for transformers. This is opt-in through `config :ash, :constraint_dependencies_from_referenced_types?`, because a type that reaches other types through its constraints without implementing `referenced_types/1` relies on the dependencies it has today. The default is intended to change in 4.0, where declaring them becomes the contract. The dependencies are registered with `Macro.compile_apply/4`, which arrived in Elixir 1.16, so the minimum is raised to match. Spark already requires 1.16, and nothing in CI set up anything older, so `~> 1.11` was a declaration alone.
a89ac1f to
f306fb1
Compare
2 participants
Contributor checklist
Leave anything that you believe does not apply unchecked.
Summary
A
constraintsvalue can hold module references with different dependency requirements:instance_ofis only read at runtime, bycast_input/2,matches_type?/2and friends.typeis initialized while the DSL is compiled, byAsh.Type.init/2fromset_type_transformation/1.Since
constraintsis unclassified, both currently create compile dependencies, so a resource recompiles whenever a struct it merely validates against changes.Which of the two a reference is, is known by the type and not by the DSL — only
Ash.Type.Structknows whatinstance_ofmeans. So instead of naming constraint keys inAsh.Resource.Dsl, this putsconstraintsinno_depend_modulesand registers the dependencies from what the type already reports:Ash.Type.constraint_referenced_types/2collects the types reachable throughc:Ash.Type.referenced_types/1— the same graphdetect_type_cycle!/2walks.Ash.Resource.Transformers.TrackConstraintTypeDependenciestraces each of them withMacro.compile_apply/4, against theMacro.EnvSpark persists as:env.instance_ofdrops out by construction:Ash.Type.Struct.referenced_types/1returnsconstraints[:fields]and notinstance_of. Third-party types get the same treatment without touching the core DSL, and fields markedinit?: falsestay excluded, matching the existing cycle walk.On a private application this cuts the recompile set for a representative resource edit from 58 files to 41.