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

Autoscoping should not escape True and False #9797

Merged

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Apr 26, 2024

Pull Request Description

close #9765

Changelog:

  • fix: do not use autoscope syntax for Boolean constructors

Important Notes

Checklist

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

  • The documentation has been updated, if necessary.
  • 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.

@4e6 4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 26, 2024
@4e6 4e6 self-assigned this Apr 26, 2024
@4e6 4e6 requested a review from jdunkerley April 26, 2024 12:24
@@ -1,3 +1,5 @@
package org.enso.compiler.test.context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

case Identifier.Qualified(name) => name.toString
case Identifier.Unqualified(name) => s"..$name"
case Identifier.Qualified(name) => name.toString
case Identifier.Unqualified("True") => "True"
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid hardcoding True and False? Cannot the builder detect that the name is actually re-exported from the library under the same name?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this smells fishy. Hardcoded values in otherwise constant-free logic.

Copy link
Contributor Author

@4e6 4e6 Apr 29, 2024

Choose a reason for hiding this comment

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

The issue was about the True and False constructors.

Cannot the builder detect that the name is actually re-exported from the library under the same name?

I'll try and see if there is enough info to implement this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot the builder detect that the name is actually re-exported from the library under the same name?

@JaroslavTulach re-exports are computed after the suggestions are built

libraryModules
.flatMap(
module -> {
var sug =
SuggestionBuilder.apply(module, getTypeHierarchy(), compiler)
.build(module.getName(), module.getIr())
.toVector()
.filter(Suggestion::isGlobal);
var exports = exportsBuilder.build(module.getName(), module.getIr());
exportsMap.addAll(module.getName(), exports);
return sug;
})
.map(
suggestion -> {
scala.collection.immutable.Set<String> identity = new ListSet<>();
var reexports =
exportsMap.get(suggestion).stream()
.map(QualifiedName::toString)
.reduce(identity, SetOps::incl, SetOps::union);
return suggestion.withReexports(reexports);
})

To make this logic work, I need to make the tag values calculation as a third step:

  • build suggestions
  • compute re-exports
  • compute tag values

I can return to this issue after the ydoc is finished

Copy link
Member

Choose a reason for hiding this comment

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

OK, feel free to integrate the "quick fix".

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label May 3, 2024
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

I don't like the fact that True and False are hardcoded... but feel free to integrate the fix.

@mergify mergify bot merged commit 6d605a5 into develop May 3, 2024
37 checks passed
@mergify mergify bot deleted the wip/db/9765-autoscoping-should-not-escape-true-and-false branch May 3, 2024 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autoscoping should not escape True and False.
3 participants