Imports conflict resolution #9093

vitvakatu · 2024-02-19T11:21:50Z

Pull Request Description

Closes #5353

When name conflict is detected, we use fully qualified name instead of the usual one.

imports.conflict.resolution.mp4

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

app/gui2/src/stores/graph/imports.ts

vitvakatu · 2024-02-19T11:53:51Z

app/gui2/src/util/ast/abstract.ts

+}
+
+/* Substitute `name` inside `expression` with `to`. */
+export function substituteQualifiedName(


There is a major flaw in the current implementation, and I’m not sure how to overcome it.
The failing test case would be:

// … const expr = Ast.parse('Name', edit) substituteQualifiedName(edit, expr, 'Name', 'OtherName' as QualifiedName)

This will not change expr, because updateValue requires some parent, but expr has none.

I overcome it by doing conflict resolution later, when we already have assignment ast: variable = expr, and in this case expr always has the parent, so updateValue works.

How it should be done?
cc @kazcw

I would separate substituteQualifiedName into two functions:

A QN operation:
getQualifiedNameSubstitution(expression: Ast, name, to): Ast.Owned | undefined

A higher-order recursive-mutation helper:
substituteRecursive(expression: MutableAst, f: (ast: MutableAst) => Owned | undefined)

If your use-case needs it, substituteRecursive could return the new Owned value if it replaces a value that had no parent--but I would expect it is possible to structure things so that that is not needed (i.e. depending on situation use getQualifiedNameSubstitution directly or use it with substituteRecursive), and that would be IMO clearer.

vitvakatu · 2024-02-19T13:19:35Z

The engine error that you see on the recording is caused by the current importing rule, which implies that the item should be exported from Main to be able to use it by a fully qualified name. So Database.Data.Table.Table needs to be exported in Standard.Database.Main. It means that the current conflict resolution will provide invalid code in a lot of cases.

app/gui2/src/components/ComponentBrowser/input.ts

farmaazon · 2024-02-19T13:51:48Z

app/gui2/src/components/ComponentBrowser/input.ts

+        const importedIdent =
+          anImport.kind == 'Qualified' ? qnLastSegment(anImport.module) : anImport.import
+        const noLongerNeeded = !code.value.includes(importedIdent)
+        if (!noLongerNeeded && !alreadyAdded) {
+          filtered.push(anImport)
+        }


I know that's an older code, but what about imports for extension methods? These imports may be from any module, whose name is not in the input at all.

app/gui2/src/components/ComponentBrowser/input.ts

farmaazon · 2024-02-19T13:57:56Z

app/gui2/src/components/ComponentBrowser/input.ts

-      const noLongerNeeded = !code.value.includes(importedIdent)
-      if (!noLongerNeeded && !alreadyAdded) {
-        finalImports.push(anImport)
+  function importsToAdd(): ImportsForEntry[] {


I don't quite like this API change. Every array element is ImportsForEntry but technically not all will contain all "Imports for entry", as the latter may have imports filtered out.

Do we use anywhere this information (i.e. which import was to what entry) outside? Maybe we could just return flat RequiredImport instead?

farmaazon · 2024-02-19T13:58:55Z

app/gui2/src/components/GraphEditor/widgets/WidgetCheckbox.vue

@@ -44,7 +44,7 @@ const value = computed({
      if (requiresImport) graph.addMissingImports(edit, theImport)
      props.onUpdate({ edit })
    } else {
-      graph.addMissingImports(edit, theImport)
+      graph.addMissingImportsDisregardConflicts(edit, theImport)


Why do we disregard conflicts here?

Fixed, I’m not sure what was the initial reasoning here.

farmaazon · 2024-02-19T14:29:40Z

app/gui2/src/stores/graph/imports.ts

+  if (!entry) return
+  const conflictingIds = suggestionDb.conflictingNames.lookup(entry.name)


It does not look like valid code. If we are about to add Table import for making Table.new node, we should look up conflicts with Table, not new.

You’re correct, it was fixed

farmaazon · 2024-02-19T14:33:43Z

app/gui2/src/components/ComponentBrowser/input.ts

+          // Keep in mind, that we need to provide the id of `entry`, not `requiredEntry` here!
+          imports.value.push({ id, imports: requiredImports(suggestionDb, requiredEntry) })


Cannot we gather here actually imported entries only? For example, when we add Table.new node: We should check conflicts for Table imports and only Table needs to be replaced with its fully qualified name, not entire chain.

Fixed, it was indeed an overcomplication of things.

farmaazon · 2024-02-19T14:35:24Z

app/gui2/src/stores/graph/imports.ts

+        const usedAs =
+          entry.memberOf != null
+            ? (`${qnLastSegment(entry.memberOf)}.${entry.name}` as QualifiedName)
+            : entry.name


What if user wrote partially qualified name? For example, Data.Vector.new, and importing Data makes conflict?

farmaazon · 2024-02-19T14:37:19Z

app/gui2/src/components/GraphEditor.vue

@@ -412,7 +412,7 @@ function hideComponentBrowser() {
  componentBrowserVisible.value = false
 }

-function onComponentBrowserCommit(content: string, requiredImports: RequiredImport[]) {
+function onComponentBrowserCommit(content: string, requiredImports: ImportsForEntry[]) {


Wait a sec, we do not add imports altogether when editing nodes! An older code too, but this time I guess we may just fix it in a separate issue.

That’s an oversight, yes. I would add it in the follow-up: #9241

farmaazon · 2024-02-20T10:23:25Z

app/gui2/src/util/ast/abstract.ts

+}
+
+/* Substitute `name` inside `expression` with `to`. */
+export function substituteQualifiedName(


This substitution won't work in cases like Foo.bar (Foo -> Foo.baz) - here we should replace only one Foo!

I thought a bit, but I don't know how to tackle it. Component Browser Input already analyses its content to know what identifiers are from outside, so it could use the information here also. On the other hand, I'm aware we also want to track import conflicts in other cases than inserting/editing node.

As discussed, we would need to refactor CB input logic to work with AST directly for this, and even then it can still be complicated.

farmaazon · 2024-03-01T14:41:42Z

app/gui2/src/components/ComponentBrowser.vue

+  const suggestionId = component?.suggestionId ?? selectedSuggestionId.value
+  if (suggestion == null || suggestionId == null) return null
+  input.applySuggestion(suggestionId)


Is here still a need for obtaining suggestion? We could just pass suggestionId and applySuggestion should handle that.

farmaazon · 2024-03-01T14:42:50Z

app/gui2/src/components/ComponentBrowser/input.ts

        if (requiredEntry) {
+          // Keep in mind, that we need to provide the id of `entry`, not `requiredEntry` here!


The comment doesn't seem to be relevant anymore.

farmaazon · 2024-03-01T14:46:07Z

app/gui2/src/stores/graph/imports.ts

+  const [entryId] = suggestionDb.nameToId.lookup(entryFQN)
+  if (entryId == null) return
+  const entry = suggestionDb.get(entryId)!
+  const conflictingIds = suggestionDb.conflictingNames.lookup(entry.name)


Technically, we could just read the last segment of entryFQN, as name, and don't fetch the entry (only entryId).

True, fixed

vitvakatu added 3 commits February 19, 2024 15:16

Implement imports conflict resolution

bc6f013

Merge branch 'develop' into wip/vitvakatu/import-conflict-resolution

32fc169

Fixes after merge

d32a7d8

vitvakatu added x-new-feature Type: new feature request -gui labels Feb 19, 2024

vitvakatu self-assigned this Feb 19, 2024

vitvakatu commented Feb 19, 2024

View reviewed changes

vitvakatu marked this pull request as ready for review February 19, 2024 11:57

vitvakatu requested review from Frizi, farmaazon, MichaelMauderer and kazcw as code owners February 19, 2024 11:57

farmaazon requested changes Feb 20, 2024

View reviewed changes

vitvakatu added 4 commits March 1, 2024 18:02

Addressing review

0ae05de

Merge branch 'develop' into wip/vitvakatu/import-conflict-resolution

45a25ee

Linting

a8eafaa

Fix docs

03f5f99

vitvakatu requested a review from farmaazon March 1, 2024 14:35

farmaazon approved these changes Mar 1, 2024

View reviewed changes

farmaazon mentioned this pull request Mar 4, 2024

Add imports on editing nodes #9253

Merged

5 tasks

vitvakatu added 3 commits March 4, 2024 14:04

Address review comments

41b7c8b

Merge branch 'develop' into wip/vitvakatu/import-conflict-resolution

20195d2

Linting

4b6690a

vitvakatu added CI: Ready to merge This PR is eligible for automatic merge CI: No changelog needed Do not require a changelog entry for this PR. labels Mar 4, 2024

mergify bot merged commit 3be5e58 into develop Mar 4, 2024
25 of 28 checks passed

mergify bot deleted the wip/vitvakatu/import-conflict-resolution branch March 4, 2024 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Imports conflict resolution #9093

Imports conflict resolution #9093

vitvakatu commented Feb 19, 2024 •

edited

Loading

vitvakatu Feb 19, 2024

kazcw Feb 19, 2024

vitvakatu commented Feb 19, 2024

farmaazon Feb 19, 2024

farmaazon Feb 19, 2024

farmaazon Feb 19, 2024

vitvakatu Mar 1, 2024

farmaazon Feb 19, 2024

vitvakatu Mar 1, 2024

farmaazon Feb 19, 2024

vitvakatu Mar 1, 2024

farmaazon Feb 19, 2024

vitvakatu Mar 1, 2024

farmaazon Feb 19, 2024

vitvakatu Mar 1, 2024

farmaazon Feb 20, 2024

vitvakatu Mar 1, 2024

farmaazon Mar 1, 2024

vitvakatu Mar 4, 2024

farmaazon Mar 1, 2024

vitvakatu Mar 4, 2024

farmaazon Mar 1, 2024

vitvakatu Mar 4, 2024

		if (!entry) return
		const conflictingIds = suggestionDb.conflictingNames.lookup(entry.name)

		// Keep in mind, that we need to provide the id of `entry`, not `requiredEntry` here!
		imports.value.push({ id, imports: requiredImports(suggestionDb, requiredEntry) })

		if (requiredEntry) {
		// Keep in mind, that we need to provide the id of `entry`, not `requiredEntry` here!

Imports conflict resolution #9093

Imports conflict resolution #9093

Conversation

vitvakatu commented Feb 19, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitvakatu commented Feb 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitvakatu commented Feb 19, 2024 •

edited

Loading