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

Patching elm/[email protected] with elm-janitor/core@084fa2b leads to a compiler error #1

Closed
marc136 opened this issue May 17, 2023 · 5 comments

Comments

@marc136
Copy link
Collaborator

marc136 commented May 17, 2023

Applying https://github.com/elm-janitor/core/tree/stack-1.0.5 with the current commit elm-janitor/core@084fa2b leads to an error during compilation:

❯ elm make src/Main.elm
Dependency problem!           
-- PROBLEM BUILDING DEPENDENCIES -----------------------------------------------

I ran into a compilation error when trying to build the following package:

    elm/core 1.0.5

This probably means it has package constraints that are too wide. It may be
possible to tweak your elm.json to avoid the root problem as a stopgap. Head
over to https://elm-lang.org/community to get help figuring out how to take this
path!

Note: To help with the root problem, please report this to the package author
along with the following information:

    

If you want to help out even more, try building the package locally. That should
give you much more specific information about why this package is failing to
build, which will in turn make it easier for the package author to fix it!
@marc136
Copy link
Collaborator Author

marc136 commented May 17, 2023

The problem seems to be this PR, no idea yet why it breaks the compilation

https://github.com/elm/core/pull/970/files

Replacing Char.toLower with toLower or Elm.Kernel.Char.toLower doesn't help.

@marc136
Copy link
Collaborator Author

marc136 commented May 21, 2023

I now narrowed the problem down to the change of isUpper and isLower inside the Char package.

Before the patch, it looks like this:

isUpper : Char -> Bool
isUpper char =
  let
    code =
      toCode char
  in
    code <= 0x5A && 0x41 <= code

and after the patch in PR 970 of elm/core it is changed to this:

isUpper : Char -> Bool
isUpper char =
  (char == Char.toUpper char)
    && (char /= Char.toLower char)

Which fails with the error shown in the first comment.

A minimal reproducible change to elm/core/1.0.5/src/Char.elm that leads to the same error is the following:

isUpper : Char -> Bool
isUpper char =
  let
    code =
      toCode char
    up =
      toUpper char
    isSame =
      up == char
  in
    code <= 0x5A && 0x41 <= code

I also tried to replace Char.toUpper with Elm.Kernel.Char.toUpper or toUpper, but I get the same result.

And I also tested it on a Macbook on the off chance that it is only my Linux system that behaves weirdly.

@marc136
Copy link
Collaborator Author

marc136 commented May 21, 2023

@rupertlssmith can you maybe test it locally on your machine if you get the same error when patching elm/core?
And if yes, can we remove the PR 970 from the stack of elm-janitor changes?

Reproduction steps:

  1. rm ~/.elm/0.19.1/packages/elm/core/1.0.5/artifacts.dat (important, otherwise the new code will not be compiled)
  2. Insert into ~/.elm/0.19.1/packages/elm/core/1.0.5/src/Char.elm inside the let block of the isUpper function
    up =
      toUpper char
    isSame =
      up == char
  1. Run rm -rf elm-stuff and elm make in any project

@rupertlssmith
Copy link
Contributor

Yes, I get the same error. That is a strange one, no idea why that change should cause that error.

I also tried with this code inserted into isUpper instead:

    _ =                                                                                                                                                                                                                                          
      char == char

Which makes me think that it is the user of == that causes the problem? Perhaps it is not defined at the point in time that Char.elm is loaded or something... unsure. I did note that == is not used elsewhere in the file.

Will link the original PR to this issue, and revert it out.

@marc136
Copy link
Collaborator Author

marc136 commented May 22, 2023

Important information from Martin Janiczek:

the "error with elm/core 1.0.5" is not the real error, did you by any chance find out what the error is when you compile elm/core with the code applied? It could be some minor syntax mistake etc ..

This lead to @rupertlssmith finding the issue by running elm make src/Char.elm inside the patched elm/core/1.0.5 directory, which then shows the actual errors:

❯ elm make src/Char.elm
Detected problems in 1 module.
-- UNKNOWN OPERATOR ----------------------------------------------- src/Char.elm

I do not recognize the (==) operator.

104|   (char == toLower char)
             ^^
Is there an `import` and `exposing` entry for it? Maybe you want (<=) or (>=)
instead?

-- UNKNOWN OPERATOR ----------------------------------------------- src/Char.elm

I do not recognize the (/=) operator.

105|     && (char /= toUpper char)
                  ^^
Is there an `import` and `exposing` entry for it? Maybe you want (<=) or (>=)
instead?

-- UNKNOWN OPERATOR ----------------------------------------------- src/Char.elm

I do not recognize the (==) operator.

84|   (char == toUpper char)
            ^^
Is there an `import` and `exposing` entry for it? Maybe you want (<=) or (>=)
instead?

-- UNKNOWN OPERATOR ----------------------------------------------- src/Char.elm

I do not recognize the (/=) operator.

85|     && (char /= toLower char)
                 ^^
Is there an `import` and `exposing` entry for it? Maybe you want (<=) or (>=)
instead?

This change fixes the issue:

- import Basics exposing (Bool, Int, (&&), (||), (>=), (<=))
+ import Basics exposing (Bool, Int, (&&), (||), (>=), (<=), (==), (/=))`

marc136 added a commit that referenced this issue May 22, 2023
marc136 added a commit that referenced this issue May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants