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

Exporting non-existing symbols fails with compiler error #7960

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Oct 3, 2023

Closes #7867

Pull Request Description

Adds a new compiler analysis pass that ensures that all the symbols exported via export ... or from ... export ... statements exist. If not, generates an IR Error.

Important Notes

We already have such a compiler pass for the version with imports in ImportSymbolAnalysis.scala

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.

@Akirathan Akirathan self-assigned this Oct 3, 2023
@Akirathan Akirathan marked this pull request as ready for review October 5, 2023 08:32
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks good

Comment on lines +81 to +86
ImportExport.apply(
exportedSymbol,
new ImportExport.SymbolDoesNotExist(exportedSymbol.name(), moduleOrTypeName.name()),
ImportExport.apply$default$3(),
ImportExport.apply$default$4()
)
Copy link
Member

Choose a reason for hiding this comment

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

It makes me sad to see this done in not-Scala, but I know this is the general direction. But it still makes me sad 🙁

Copy link
Member Author

Choose a reason for hiding this comment

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

Apart from some weird perks, like calling methods with default arguments, or methods on objects, there is nothing stopping us from calling Scala stuff from Java stuff anymore. It does not look nice, but it is the most viable compromise for now.

@@ -175,7 +175,6 @@ from project.Data.Numbers export Float, Integer, Number
from project.Data.Range.Extensions export all
from project.Data.Statistics export all hiding to_moment_statistic, wrap_java_call, calculate_correlation_statistics, calculate_spearman_rank, calculate_correlation_statistics_matrix, compute_fold, empty_value, is_valid
from project.Data.Text.Extensions export all
from project.Data.Time.Conversions export all
Copy link
Member

Choose a reason for hiding this comment

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

This file does not exist, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yep.

It's my fault, I forgot to remove this export after removing the file, sorry.

Thanks for adding a pass that catches such errors! ❤️

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly. This is why it is now removed - the compilation now fails with this pass.

@Akirathan Akirathan added CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Keep up to date Automatically update this PR to the latest develop. labels Oct 5, 2023
@Akirathan Akirathan removed the CI: Keep up to date Automatically update this PR to the latest develop. label Oct 5, 2023
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.

Nice to see we detected an error in standard library.

private UUID uuid;

private PrivateModuleAnalysis() {}

public static PrivateModuleAnalysis getInstance() {
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
Member Author

Choose a reason for hiding this comment

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

In most scenarios, I would say so. But here, I wanted to get as closest to scalac as possible - which creates $MODULE$ public static final field for object. Just my preference. I could refactor it to getter once more.

JaroslavTulach and others added 2 commits October 5, 2023 18:34
…d-result-in-an-error' of github.com:enso-org/enso into wip/akirathan/7867-Exporting-non-existing-symbols-should-result-in-an-error
@Akirathan Akirathan added the CI: Keep up to date Automatically update this PR to the latest develop. label Oct 6, 2023
@Akirathan Akirathan added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Keep up to date Automatically update this PR to the latest develop. labels Oct 6, 2023
@enso-bot
Copy link

enso-bot bot commented Oct 6, 2023

Pavel Marek reports a new STANDUP for yesterday (2023-10-05):

Progress: - Finalizing the PR

  • Fighting with the CI It should be finished by 2023-10-09.

@mergify mergify bot merged commit 4db6121 into develop Oct 9, 2023
31 checks passed
@mergify mergify bot deleted the wip/akirathan/7867-Exporting-non-existing-symbols-should-result-in-an-error branch October 9, 2023 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exporting non-existing symbols should result in an error
4 participants