Skip to content

Deprecate primes in identifiers exported from foreign modules #3792

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.

Sign up for GitHub

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

Jump to bottom
Merged
hdgarrood merged 1 commit into from
Apr 4, 2020
Merged

Deprecate primes in identifiers exported from foreign modules #3792

hdgarrood merged 1 commit into from
Apr 4, 2020

Conversation

@kl0tl
Copy link
Member

@kl0tl kl0tl commented Feb 16, 2020

This PR implements @hdgarrood suggestion regarding the depreciation of primes in foreign modules exports.

natefaubion
natefaubion reviewed Feb 22, 2020
View reviewed changes
src/Language/PureScript/CodeGen/JS.hs Outdated
(AST.StringLiteral Nothing $ mkString (T.replace "'" "$prime" unescaped)))
(AST.Var Nothing "$foreign"))
| otherwise
= accessorString (mkString unescaped) (AST.Var Nothing "$foreign")
Copy link
Contributor

@natefaubion natefaubion Feb 22, 2020

Choose a reason for hiding this comment

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

Is this emitted for every reference to a foreign identifier within a module?

Copy link
Member Author

@kl0tl kl0tl Feb 23, 2020

Choose a reason for hiding this comment

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

$foreign[$foreign.hasForeignProperty("foo'") ? "foo'" : "foo$prime"] would have been emitted for every reference to a foo' identifier imported from a foreign module yes.

@natefaubion
Copy link
Contributor

natefaubion commented Feb 22, 2020

Personally, I'm inclined to remove the hasOwnProperty check and just disallow foreign primes entirely rather than suggest you use $prime escaping.

@hdgarrood
Copy link
Contributor

hdgarrood commented Feb 22, 2020

This looks like a great start, thank you! I think it might be preferable to use the existing IfElse rather than using a new Ternary constructor though (it slipped my mind that we don't already have one).

@hdgarrood
Copy link
Contributor

hdgarrood commented Feb 22, 2020

Personally, I'm inclined to remove the hasOwnProperty check and just disallow foreign primes entirely rather than suggest you use $prime escaping.

So I guess we would spend a major release series warning on FFI primes, and where the way of silencing the warning is to rename to something which doesn't contain primes at all, and then make it an error when we do ES modules? I guess that's preferable in that it doesn't involve messing with the generated code or purs bundle at all.

@hdgarrood
Copy link
Contributor

hdgarrood commented Feb 22, 2020

Also it means that people are less likely to accidentally break their libraries on older compiler versions when dealing with the warning, which is probably also a plus.

@hdgarrood hdgarrood added this to the v0.14.0 milestone Feb 22, 2020
@kl0tl kl0tl force-pushed the deprecated-ffi-primes branch from 8d7996a to cf33e3f Compare February 23, 2020 10:31
@kl0tl kl0tl changed the title Deprecate unescaped primes in identifiers exported from foreign modules Deprecate primes in identifiers exported from foreign modules Feb 23, 2020
@kl0tl
Copy link
Member Author

kl0tl commented Feb 23, 2020

I’ve removed the support for escaped primes and updated the deprecation message.

@kl0tl kl0tl force-pushed the deprecated-ffi-primes branch from cf33e3f to 28b5645 Compare February 23, 2020 10:51
This was referenced Feb 23, 2020
Closed
Merged
hdgarrood
hdgarrood reviewed Mar 5, 2020
View reviewed changes
tests/purs/warning/DeprecatedFFIPrime.out Outdated

In the FFI module for Main:

The identifier a' contains a '.
Copy link
Contributor

@hdgarrood hdgarrood Mar 5, 2020

Choose a reason for hiding this comment

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

I think people might not realise that a ' is the same thing as a prime, which might make this wording confusing. How about having the error say this:

The identifier a' contains a prime (').
Primes in identifiers ...

Copy link
Member Author

@kl0tl kl0tl Mar 7, 2020

Choose a reason for hiding this comment

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

I applied your suggestion!

@kl0tl kl0tl force-pushed the deprecated-ffi-primes branch from 28b5645 to 9e7e937 Compare March 7, 2020 12:54
hdgarrood
hdgarrood reviewed Mar 7, 2020
View reviewed changes
Copy link
Contributor

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! Could you add yourself to CONTRIBUTORS.md too please?

@kl0tl kl0tl force-pushed the deprecated-ffi-primes branch from 9e7e937 to d28a891 Compare March 7, 2020 13:40
@kl0tl
Copy link
Member Author

kl0tl commented Mar 7, 2020

Sorry for missing that, I just did.

@hdgarrood
Copy link
Contributor

hdgarrood commented Mar 7, 2020

No worries - thanks very much!

This was referenced Mar 7, 2020
Merged
Merged
Merged
Merged
Merged
@kl0tl
Copy link
Member Author

kl0tl commented Mar 7, 2020

I opened a few pull requests removing primes from foreign modules on the libraries I had to patch to be able to run tests while working on #3791. There’s probably a lot more libraries to upgrade in the official package set but that’s a start 😅

@garyb
Copy link
Member

garyb commented Mar 9, 2020

Thanks! I'll go through and get those all merged before we make the new release.

This was referenced Mar 13, 2020
Merged
Merged
Closed
Merged
This was referenced Mar 13, 2020
Merged
Merged
Merged
Merged
Merged
@kl0tl kl0tl force-pushed the deprecated-ffi-primes branch 2 times, most recently from a4cfcd9 to dff3f62 Compare March 13, 2020 18:11
@kl0tl
Copy link
Member Author

kl0tl commented Mar 13, 2020

I rebased the PR onto master (SimpleErrorMessage moved from Language.PureScript.AST.Declarations to Language.PureScript.Errors) and opened pull requests on all other packages impacted by the depreciation in the set psc-0.13.6-20200309!

@kl0tl kl0tl force-pushed the deprecated-ffi-primes branch from dff3f62 to aa8a4e5 Compare March 29, 2020 17:39
@hdgarrood
Copy link
Contributor

hdgarrood commented Apr 1, 2020

Any objections to merging this now?

@natefaubion
Copy link
Contributor

natefaubion commented Apr 1, 2020

No objections from me.

@hdgarrood hdgarrood merged commit 1fcbb06 into purescript:master Apr 4, 2020
@kl0tl kl0tl deleted the deprecated-ffi-primes branch April 13, 2020 16:47
@joneshf joneshf mentioned this pull request May 23, 2020
Closed
@paluh paluh mentioned this pull request Jul 5, 2020
Closed
@kl0tl kl0tl mentioned this pull request Jul 19, 2020
Merged
@JordanMartinez JordanMartinez mentioned this pull request Jan 18, 2022
Closed
@thomashoneyman thomashoneyman mentioned this pull request Jan 29, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@natefaubion natefaubion natefaubion left review comments

+1 more reviewer

@hdgarrood hdgarrood hdgarrood approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v0.14.0

Development

Successfully merging this pull request may close these issues.

4 participants

@kl0tl @natefaubion @hdgarrood @garyb