feat(libflux): associate registry attributes to import statements #5332
Conversation
jsternberg
force-pushed
the
feat/module-imports
branch
3 times, most recently
from
e6bd4c6
to
e36fe10
Compare
| if p.is_empty() || p == "." || p == ".." { | ||
| self.errors.push(located( | ||
| path.base.location.clone(), | ||
| ErrorKind::InvalidImportPath(path_str.to_string()), |
There was a problem hiding this comment.
This could explain why the path is invalid more precisely, either by pointing precisely to the invalid segment, or explaining why a segment is invalid.
There was a problem hiding this comment.
I'll give this a try.
There was a problem hiding this comment.
Ok I changed the error message to be specific about which path segment. I couldn't really get it to point to the exact location, but it should have the location of the import itself and the path segment that was invalid.
| @@ -610,7 +610,8 @@ pub struct ImportDeclaration { | |||
| pub loc: ast::SourceLocation, | |||
|
|
|||
| pub alias: Option<Identifier>, | |||
| pub path: StringLit, | |||
| pub registry: Option<String>, | |||
There was a problem hiding this comment.
Nit: This could maybe be an enum to make it clearer
enum Registry {
Stdlib,
Other(String),
}
There was a problem hiding this comment.
I like this change, but just wanted to note that this isn't going to end up influencing the flatbuffers representation. I think it adds clarity to the Rust code but it doesn't really add anything to flatbuffers in my opinion and the flatbuffers representation will likely be removed if we refactor the execution to happen in Rust anyway so I don't want to spend too much time on adding an enum to flatbuffers.
There was a problem hiding this comment.
I was trying to integrate this and I think I'll just merge the current version and we can reconsider this in the future.
The reason is because I started to make the change and thought, "Well the standard library isn't really a registry. So let me use a different name." Then I started going down a rabbit hole of changing names and I'd like to get this merged to unblock other work.
There was a problem hiding this comment.
Yeah, I don't think this needs to be represented in the flatbuffer, just would be nice to be clearer on the rust side.
| As *Identifier | ||
| Path *StringLiteral | ||
| As *Identifier | ||
| Registry *string |
There was a problem hiding this comment.
Nit: When resolving the import here on the go side you could add an error case for when a registry exist, to serve as a point to fill in the actual implementation later if nothing else.
There was a problem hiding this comment.
This is going to follow as the next PR for the Go side.
jsternberg
force-pushed
the
feat/module-imports
branch
2 times, most recently
from
7f82391
to
886f414
Compare
jsternberg
force-pushed
the
feat/module-imports
branch
2 times, most recently
from
54df263
to
51734b8
Compare
This change retrieves the `@registry` attribute from the package and associates those definitions with corresponding imports in the same package. This gets encoded in the semantic graph and the flatbuffer so it is accessible in both Rust and Go.
jsternberg
force-pushed
the
feat/module-imports
branch
from
51734b8
to
8ea7a67
Compare
…lsa query (#5324)" (#5342) * fix: revert "feat: Download Flux modules within runtime as part of Salsa query (#5324)" This reverts commit cced93e. The commit broke nightlies. * fix: revert "feat(libflux): associate registry attributes to import statements (#5332)" This reverts commit 77b1d78. * fix: revert "refactor: Remove flux_analyze (#5335)" This reverts commit 1db3400.
This change retrieves the
@registryattribute from the package andassociates those definitions with corresponding imports in the same
package.
This gets encoded in the semantic graph and the flatbuffer so it is
accessible in both Rust and Go.
Fixes #5311.
Checklist
Dear Author 👋, the following checks should be completed (or explicitly dismissed) before merging.
experimental/docs/Spec.mdhas been updatedDear Reviewer(s) 👋, you are responsible (among others) for ensuring the completeness and quality of the above before approval.