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

Add (++) for List variation of All #3179

Merged
merged 9 commits into from
Jan 2, 2024

Conversation

joelberkeley
Copy link
Contributor

Description

This exists in Data for SnocList and Vect but not for List for some reason. I copied it verbatim from src

Should this change go in the CHANGELOG?

  • If this is a fix, user-facing change, a compiler change, or a new paper
    implementation, I have updated CHANGELOG.md (and potentially also
    CONTRIBUTORS.md).

@joelberkeley joelberkeley changed the title List all append Add (++) for All for List Dec 30, 2023
@joelberkeley joelberkeley changed the title Add (++) for All for List Add (++) for List variation of All Dec 30, 2023
@joelberkeley
Copy link
Contributor Author

joelberkeley commented Dec 30, 2023

I don't understand why this is failing. It's unable to disambiguate between the version in src and that I've added here in base. But src isn't imported in base. More importantly, src isn't importable by clients of base afaik, so I'd like to put this in base

@mattpolzin
Copy link
Collaborator

Correct that src isn't imported into base, but rather the other way around: base is imported into the compiler source code -- specifically on a module by module basis, but the module of base that you added the new function to is imported upstream of the locations where ambiguity is occurring.

This is a good PR IMO, but you'll need to explicitly use the library definition of (++) in the source code to disambiguate for now. Once we release the next version of the compiler, it will be safe to bump the minimum required compiler version and remove the src version of the function in favor of the base version.

@mattpolzin
Copy link
Collaborator

I'd leave a note in the compiler source code about the future desire to remove the src version of the function (after the next release).

@joelberkeley
Copy link
Contributor Author

thank @mattpolzin, is the current compiler version 0.7.0?

@mattpolzin
Copy link
Collaborator

is the current compiler version 0.7.0?

Yeah. And for reference, this is the sort of TODO that I'd leave for future us: https://github.com/idris-lang/Idris2/pull/3174/files#diff-458f49aa263db28859e440611f0e9e49e4cbbef7393c4215580029d938980c24L5

@joelberkeley
Copy link
Contributor Author

all done

@mattpolzin
Copy link
Collaborator

You actually won't need to disambiguate where you did in the latest changes. Within base, this is the only (++) for the List.Quantifiers.All scope. You'll need to make a similar change within src (https://github.com/idris-lang/Idris2/blob/main/src/Libraries/Data/List/Quantifiers/Extra.idr#L20), though. That's where you need to indicate that the compiler code should be using Extra.(++) for now.

@@ -14,10 +14,12 @@ lookup v (px :: pxs)
No _ => lookup v pxs
Yes Refl => Just px

-- TODO: delete this function once we release the next compiler version
-- (after 0.7.0) as it has been replicated in base's Data.List.Quantifiers
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please change the wording to something like "delete this function after 0.7.1 is released" because wording "next release after 0.7.0" proved to be really confusing (well, at least for me, doing cleanups). I think wording mentioning 0.7.1 is okay even if next release would be 0.8.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. All good?

Copy link
Collaborator

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattpolzin mattpolzin merged commit ee18555 into idris-lang:main Jan 2, 2024
22 checks passed
@joelberkeley joelberkeley deleted the list-all-append branch January 2, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants