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

Methods for bits to union variant and back #49298

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Tokazama
Copy link
Contributor

@Tokazama Tokazama commented Apr 9, 2023

Implements tag_to_variant, variant_to_tag, and renames/optimizes unionlen to nvariants. This is only intended to provide some rudimentary tools for working with Union and perhaps normalize some of the language used for referring to Union related stuff moving forward. Hopefully this will make related work a bit easier to discuss and test going forward.

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

What is the long-term purpose of this change? I am mildly against renamings for renamings sake, and mildly against adding internal helper functions that have no use.

@Tokazama
Copy link
Contributor Author

Tokazama commented Apr 9, 2023

What is the long-term purpose of this change? I am mildly against renamings for renamings sake, and mildly against adding internal helper functions that have no use.

The rename is for consistency with a the proposed terminology. I figured it was better to be consistent than avoid renaming an internal method, but I can change it back if it's a bad idea.

It doesn't have any internal application at this time but these are some of the most basic methods necessary for working with the Union type. I thought it would make more sense to start with something small and terminology focused before anyone starts seriously working on any other union related stuff.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 9, 2023

What Union-related stuff would use this?

@mkitti
Copy link
Contributor

mkitti commented Apr 9, 2023

Wouldn't it be nice to at least offer a deprecation warning?

I recommend splitting this into parts:

  1. New and extended implementation of unionlen without any renaming
  2. Introduction of nvariants alias to unionlen
  3. Deprecation of unionlen in favor of nvariants
  4. Removal of unionlen

@Tokazama
Copy link
Contributor Author

What Union-related stuff would use this?

Well, there's several issues off the top of my head that are either related to traversal of Union directly or terminology around tagged types #47735, #48883, #37790.

I wrote it originally as part of #48728 where it's necessary to traverse Union in order to lower element retrieval into native Julia code.

If it's 100% the wrong move to include this I'm not going to fight it tooth and nail but I do think we eventually want to move a lot of C code to Julia code and to quote @KristofferC, "It's always good to get stuff from C into Julia."

Wouldn't it be nice to at least offer a deprecation warning?

Deprecation is now included.

@Tokazama
Copy link
Contributor Author

Tokazama commented Apr 10, 2023

Alternatively, would it make sense to treat Union like a collection and use Base.length and indexing? I figured it made sense to have distinct methods for work with the type system but it might be convenient to just treat it like a collection of types.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 10, 2023

No, types should not be thought of as being generically indexable. Similar ideas have been proposed before and rejected, since we don't really want people to rely on that (e.g. it changes pretty often internally).

@mkitti
Copy link
Contributor

mkitti commented Apr 10, 2023

Base.length seems to make sense though, no? Does that imply indexing?

@Tokazama
Copy link
Contributor Author

Tokazama commented Apr 10, 2023

I opened up the possibility of treating it like a collection because I wanted to make sure we covered our bases, but Union is intended to represent a single instance instead of a collection. I think treating it as a collection is erroneously conflating its implementation with its usage. But I didn't invent the type system so I ultimately defer to others.

@Tokazama
Copy link
Contributor Author

since we don't really want people to rely on that (e.g. it changes pretty often internally).

This is one of reasons we should have more formality in how we refer to and interact with Union. People seem to be using unexported and undocumented methods or rolling their own code based on the current implementation of Union, which may be subject to change in the future.

With that in mind, it might be better for this to move away from directly using tag so that dynamically identifying the variant type is less directly tied to the the underlying structure of Union.

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

Successfully merging this pull request may close these issues.

3 participants