Skip to content
This repository has been archived by the owner on Dec 28, 2021. It is now read-only.

Argument Names in the Span Tree #781

Merged
merged 38 commits into from
Sep 11, 2020
Merged

Argument Names in the Span Tree #781

merged 38 commits into from
Sep 11, 2020

Conversation

mwu-tow
Copy link
Contributor

@mwu-tow mwu-tow commented Sep 7, 2020

Pull Request Description

This PR adds information about argument names and types to the span tree nodes.

The arguments are recognized based on the intended method metadata entry (if present and name matches) or the computed value registry.

This is feature complete PR. Still more tests are intended.

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Rust.
  • All code has been tested where possible.
  • All code has been profiled where possible.

@mwu-tow mwu-tow marked this pull request as draft September 7, 2020 01:36
@farmaazon farmaazon added this to the 31-08-2020 milestone Sep 7, 2020
src/rust/ide/lib/span-tree/src/generate.rs Outdated Show resolved Hide resolved
src/rust/ide/lib/span-tree/src/generate.rs Outdated Show resolved Hide resolved
src/rust/ide/lib/span-tree/src/lib.rs Show resolved Hide resolved
src/rust/ide/lib/span-tree/src/generate/context.rs Outdated Show resolved Hide resolved
Comment on lines +149 to +161
let chain = infix.flatten();
let app_base = ApplicationBase::new(self);
let invocation = || -> Option<CalledMethodInfo> {
context.call_info(self.id?, app_base.function_name)
}();

// All prefix params are missing arguments, since there is no prefix application.
let missing_args = app_base.prefix_params(invocation);
let arity = missing_args.len();
let base_node_kind = if arity==0 {kind} else {node::Kind::Operation};
let node = chain.generate_node(base_node_kind,context)?;
let provided_prefix_arg_count = 0;
Ok(generate_expected_arguments(node,kind,provided_prefix_arg_count,missing_args))
Copy link
Collaborator

Choose a reason for hiding this comment

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

The infix could be a function part of prefix chain - and possibly we could generate ports for arguments twice (all depends of definition of "expression being a call". Let's discuss this case on the call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving as-is for now, as discussed.
Current language server behavior is that it provides a method pointer only for fully saturated calls.

Comment on lines 510 to 524
///
/// Will use `self` as the context for span tree generation.
pub fn connections_unevaluated_ctx(&self) -> FallibleResult<Connections> {
let graph = self.graph_info()?;
Ok(Connections::new(&graph,self))
}

/// Returns information about all the connections between graph's nodes.
pub fn connections
(&self, context:&impl SpanTreeContext) -> FallibleResult<Connections> {
let graph = self.graph_info()?;
Ok(Connections::new(&graph))
// TODO perhaps this should merge given context with the metadata information
// or perhaps this should just do exactly what it is told
Ok(Connections::new(&graph,context))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would make functions:

  • connections(self) - to provide connections with data from metadata
  • connections_enriched(self, context) - to apply result of context before metadata - being used by executed graph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving as-is, as discussed. For now that for consistency would require providing a number of mostly duplicated unused methods.
If ever needed, then they can provided or alternatively a design where graph controller holds context as a field should be reconsidered.

src/rust/ide/lib/span-tree/src/generate/context.rs Outdated Show resolved Hide resolved
src/rust/ide/lib/span-tree/src/generate/context.rs Outdated Show resolved Hide resolved
src/rust/ide/src/controller/graph.rs Outdated Show resolved Hide resolved
src/rust/ide/src/controller/graph/executed.rs Show resolved Hide resolved
src/rust/ide/src/controller/module.rs Outdated Show resolved Hide resolved
src/rust/ide/src/controller/module.rs Outdated Show resolved Hide resolved
src/rust/ide/src/model/project.rs Outdated Show resolved Hide resolved
src/rust/ide/src/model/module/plain.rs Outdated Show resolved Hide resolved
src/rust/ide/src/test.rs Outdated Show resolved Hide resolved
mwu-tow and others added 8 commits September 10, 2020 14:02
@mwu-tow mwu-tow changed the title [WIP] Argument Names in the Span Tree Argument Names in the Span Tree Sep 11, 2020
@mwu-tow mwu-tow marked this pull request as ready for review September 11, 2020 01:54
@mwu-tow mwu-tow merged commit 04879bd into main Sep 11, 2020
@mwu-tow mwu-tow deleted the wip/mwu/arg-names branch September 11, 2020 16:09
mwu-tow added a commit to enso-org/enso that referenced this pull request Oct 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants