loading: Try to clean up extension loading a bit #59863
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The loading code is overall a bit messy. I'd like to add some new features to it, but I think before I can sensibly do that, some cleanup is required. This attempts to de-deduplicate the several places where extensions deal with weakdeps. The logic for extension loading weakdeps is present multiple times because we support loading extension: 1. By looking at the [extensions] section of the currently active Project.toml (for loading extensions of the currently active top level project) 2. By looking at the [extensions] sub-section of the top-level Manifest.toml (for any extension of a non-toplevel package); and 3. Like 1, except that rather than the env specifying a toplevel Project.toml, it specifies a directory and we search the second level for a Project.toml. This separation is a sensible resolution to the tradeoff of not wanting to scan all package's `Project.toml`s at load time while also not forcing a `resolve` for changes to the top-level Project.toml. Unfortunately, the loading behavior in case 1 currently differs from the loading behavior of case 2 and 3. In case 1, an extension is allowed to load any weakdep, in cases 2/3, it's only allowed to load weakdeps that are also extension triggers. I believe that the missing extra check for this restriction in case 1 is an oversight. So, this refactors all that by adding the check to case 1 and mostly deleting the code for case 3, by just putting the project-file-lookup code inline at the start of case 1. Additionally, I've factored out common queries into utility functions, even when the code itself is simple. The idea is to signal that these are the same data structure even if they appear in different places (Project vs Manifest). Lastly, this makes the following additional behavior changes that I don't expect to be observable: In case 2, when loading a non-extension-trigger, don't restart the search from scratch. The only case in which I could see this causing an observable behavior difference is if there are multiple versions of the package in the load path and for some reason the highest priority one doesn't have the extension (but then how did you load it in the first place??). However, even in that case, I think the new behavior is better, because it would get the dep of the version that *did* declare the extension.
With this, do you mean what was added in #53314?
Yes, that sounds wrong to me. |
KristofferC
reviewed
Oct 16, 2025
| return nothing | ||
| end | ||
|
|
||
| dep_stanza_get(stanza::Nothing, name::String) = nothing |
There was a problem hiding this comment.
Personally, I like dealing with cases like this inline instead of having a full method definition for it.
There was a problem hiding this comment.
I usually think that's reasonably, but in this case, the calling function is already super complicated and I think it's nice not to complicate it further with the extra check. Shouldn't make a difference to code generation since it should union split and inline equivalent code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The loading code is overall a bit messy. I'd like to add some new features to it, but I think before I can sensibly do that, some cleanup is required. This attempts to de-deduplicate the several places where extensions deal with weakdeps. The logic for extension loading weakdeps is present multiple times because we support loading extension:
By looking at the [extensions] section of the currently active Project.toml (for loading extensions of the currently active top level project)
By looking at the [extensions] sub-section of the top-level Manifest.toml (for any extension of a non-toplevel package); and
Like 1, except that rather than the env specifying a toplevel Project.toml, it specifies a directory and we search the second level for a Project.toml.
This separation is a sensible resolution to the tradeoff of not wanting to scan all package's
Project.tomls at load time while also not forcing aresolvefor changes to the top-level Project.toml.Unfortunately, the loading behavior in case 1 currently differs from the loading behavior of case 2 and 3. In case 1, an extension is allowed to load any weakdep, in cases 2/3, it's only allowed to load weakdeps that are also extension triggers. I believe that the missing extra check for this restriction in case 1 is an oversight.
So, this refactors all that by adding the check to case 1 and mostly deleting the code for case 3, by just putting the project-file-lookup code inline at the start of case 1.
Additionally, I've factored out common queries into utility functions, even when the code itself is simple. The idea is to signal that these are the same data structure even if they appear in different places (Project vs Manifest).
Lastly, this makes the following additional behavior changes that I don't expect to be observable:
In case 2, when loading a non-extension-trigger, don't restart the search from scratch. The only case in which I could see this causing an observable behavior difference is if there are multiple versions of the package in the load path and for some reason the highest priority one doesn't have the extension (but then how did you load it in the first place??). However, even in that case, I think the new behavior is better, because it would get the dep of the version that did declare the extension.