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

feat(modules): add import!() expression #898

Merged
merged 1 commit into from
Sep 8, 2023
Merged

Conversation

zzlk
Copy link
Contributor

@zzlk zzlk commented Sep 6, 2023

No description provided.

Copy link
Member

@MingweiSamuel MingweiSamuel left a comment

Choose a reason for hiding this comment

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

Don't want to use the mod syntax instead of input and output? (Also, input and output should always be reserved even if you're not in a module)

@zzlk
Copy link
Contributor Author

zzlk commented Sep 6, 2023

Don't want to use the mod syntax instead of input and output? (Also, input and output should always be reserved even if you're not in a module)

I had some trouble getting it to parse because mod is a keyword rather than ident. I can look into it again now though.

I agree that whatever thing is used it should be reserved inside and outside modules.

@zzlk zzlk force-pushed the imports branch 3 times, most recently from 0476901 to 3ed703e Compare September 6, 2023 23:00
@zzlk
Copy link
Contributor Author

zzlk commented Sep 6, 2023

Some things that are not yet done (I think these will likely turn into github issues that we can address later).

  1. Nice boundaries in the mermaid when there are module boundaries.
  2. What to do about port names into/outof modules for mermaid? Currently they are just deleted.
  3. Better error handling although that likely needs some rust changes.
  4. Templating/arguments for modules at compile time.
  5. Likely need an enormous number of compile-fail tests to make sure the errors are good...
  6. make merge_modules happen during import processing rather than as a dedicated post-processing step on the flat graph.

@zzlk zzlk force-pushed the imports branch 3 times, most recently from 0120512 to a5dc7b9 Compare September 7, 2023 00:22
@zzlk zzlk requested a review from MingweiSamuel September 7, 2023 16:10
hydroflow/examples/modules/triple_cross_join.hf Outdated Show resolved Hide resolved
hydroflow_lang/src/graph/di_mul_graph.rs Outdated Show resolved Hide resolved
hydroflow_lang/src/graph/flat_graph_builder.rs Outdated Show resolved Hide resolved
hydroflow_lang/src/graph/flat_graph_builder.rs Outdated Show resolved Hide resolved
hydroflow_lang/src/graph/flat_graph_builder.rs Outdated Show resolved Hide resolved
hydroflow_lang/src/graph/flat_graph_builder.rs Outdated Show resolved Hide resolved
hydroflow_lang/src/graph/hydroflow_graph.rs Outdated Show resolved Hide resolved
Comment on lines +268 to +267
self.merge_in(flat_graph, import.span())
}
Copy link
Member

Choose a reason for hiding this comment

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

Call merge_modules here instead? And/or move that code here/into FlatGraphBuilder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think name resolution needs to happen before merge_modules is called.

if !diagnostics.iter().any(Diagnostic::is_error) {
flat_graph.merge_modules();
Copy link
Member

Choose a reason for hiding this comment

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

Call merge_modules be called right when the import is handled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it needs to happen after name resolution.

let flat_graph_builder = FlatGraphBuilder::from_hfcode(input, macro_invocation_path);
let (mut flat_graph, _uses, diagnostics) = flat_graph_builder.build();
if !diagnostics.iter().any(Diagnostic::is_error) {
flat_graph.merge_modules();
Copy link
Member

Choose a reason for hiding this comment

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

.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what does the dot mean?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, also could move merge_modules out of here if it is done automatically

Comment on lines +369 to +378
impl Display for PortIndexValue {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
PortIndexValue::Int(x) => write!(f, "{}", x.to_token_stream().to_string()),
PortIndexValue::Path(x) => write!(f, "{}", x.to_token_stream().to_string()),
PortIndexValue::Elided(_) => write!(f, "[]"),
}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Where is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used when outputting an error message about incorrect ports. It's in remove_module_boundary

@zzlk zzlk merged commit 6e8fba7 into hydro-project:main Sep 8, 2023
11 checks passed
nickjiang2378 pushed a commit to nickjiang2378/hydroflow that referenced this pull request Jan 24, 2024
nickjiang2378 pushed a commit to nickjiang2378/hydroflow that referenced this pull request Jan 25, 2024
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.

2 participants