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

Track quality of jkinds #3505

Open
wants to merge 16 commits into
base: aspsmith/jkind-in-types
Choose a base branch
from

Conversation

glittershark
Copy link
Member

Sometimes, inferred jkinds for a type are not exact, but instead upper bounds;
later, we might learn more about a type that causes us to lower that jkind
bound. For example, we might substitute a type variable, or perform with
substitution on an abstract type in a module type. It's important to know
whether this might happen, so that we can avoid normalizing away types with
jkinds that might get "better" later from with-bounds during jkind
normalization.

This commit adds a new "jkind_quality" type, and a "quality" field to jkind,
that tracks whether an inferred jkind is "best" or "not best", and sets this to
the correct value everywhere jkinds are constructed. Nothing actually /reads/
this field yet, but this will be used during normalization in a subsequent
commit.

@glittershark glittershark force-pushed the aspsmith/jkind-quality branch from f7e73cf to 4636749 Compare January 24, 2025 14:41
@glittershark glittershark added typing modes Work on modes. There's some overlap with the `multicore` label, but not strictly so. labels Jan 24, 2025
@glittershark glittershark force-pushed the aspsmith/jkind-in-types branch from 9fb7e36 to f2d1d14 Compare January 24, 2025 18:56
@glittershark glittershark force-pushed the aspsmith/jkind-quality branch from 4636749 to 5c180a9 Compare January 24, 2025 18:58
@glittershark glittershark force-pushed the aspsmith/jkind-in-types branch from f2d1d14 to 5e3b03c Compare January 24, 2025 20:15
@glittershark glittershark force-pushed the aspsmith/jkind-quality branch 2 times, most recently from 719d602 to d3b1f27 Compare January 28, 2025 16:08
Sometimes, inferred jkinds for a type are not exact, but instead *upper bounds*;
later, we might learn more about a type that causes us to lower that jkind
bound. For example, we might substitute a type variable, or perform `with`
substitution on an abstract type in a module type. It's important to know
whether this might happen, so that we can avoid normalizing away types with
jkinds that might get "better" later from with-bounds during jkind
normalization.

This commit adds a new "jkind_quality" type, and a "quality" field to jkind,
that tracks whether an inferred jkind is "best" or "not best", and sets this to
the correct value everywhere jkinds are constructed. Nothing actually /reads/
this field yet, but this will be used during normalization in a subsequent
commit.
@glittershark glittershark force-pushed the aspsmith/jkind-quality branch from d3b1f27 to 523ec09 Compare January 28, 2025 16:35
typing/types.mli Show resolved Hide resolved
typing/jkind.mli Outdated Show resolved Hide resolved
typing/jkind.mli Show resolved Hide resolved
typing/jkind.ml Show resolved Hide resolved
typing/typedecl.ml Outdated Show resolved Hide resolved
typing/predef.ml Show resolved Hide resolved
typing/jkind.ml Outdated Show resolved Hide resolved
typing/jkind.ml Show resolved Hide resolved
typing/jkind.ml Outdated Show resolved Hide resolved
typing/jkind.ml Outdated Show resolved Hide resolved
@goldfirere
Copy link
Collaborator

Reviewed two commits ("add some extra comments from review".."more comments, specifically documenting quality"). They look good to me.

Copy link
Collaborator

@goldfirere goldfirere left a comment

Choose a reason for hiding this comment

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

Checked the most recent commits ("Define the jkind of or_null once, rather than redefining in typedecl".."simplify best jkinds in predef for type1s", "format").

typing/typedecl.ml Outdated Show resolved Hide resolved
typing/jkind.ml Outdated Show resolved Hide resolved
it's weird, and we don't need it, so let's just remove it
typing/typedecl.ml Outdated Show resolved Hide resolved
@goldfirere
Copy link
Collaborator

Reviewed latest commits. This one looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modes Work on modes. There's some overlap with the `multicore` label, but not strictly so. typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants