Skip to content

Generics overhaul & improvements #2324

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

Merged
merged 47 commits into from
May 26, 2025

Conversation

FnControlOption
Copy link
Contributor

@FnControlOption FnControlOption commented May 16, 2025

Closes #1601
Closes #2274

  • Deleted Analyser.BoundTypeParams
  • Added Analyser.ResolveOptions, which contains a node + handle and an optional field for container_type
    • If present, the container_type is used instead of calling Analyser.innermostContainer(). This is used by functions, fields, and @This().
  • Added some things to Analyser.Type.Data
    • Container struct. This contains the container scope + handle and an ArrayHashMap(TokenWithHandle, Analyser.Type) for bound type parameters.
    • generic field. This is just a token + handle. (Important: not a string, to avoid conflicts between functions that have the same parameter names)
    • isGeneric()/resolveGeneric() methods
  • Passes all tests except for "usingnamespace - generics" (originally part of "usingnamespace")

@FnControlOption FnControlOption deleted the wip-generics branch May 16, 2025 23:27
@FnControlOption

This comment was marked as resolved.

@FnControlOption FnControlOption restored the wip-generics branch May 16, 2025 23:28
@FnControlOption FnControlOption changed the title Overhaul analysis code for generics Generics improvements May 18, 2025
@FnControlOption FnControlOption changed the title Generics improvements Generics overhaul & improvements May 18, 2025
@FnControlOption FnControlOption marked this pull request as draft May 18, 2025 19:20
@FnControlOption FnControlOption marked this pull request as ready for review May 18, 2025 21:09
@FnControlOption
Copy link
Contributor Author

Would it be preferred to break this pull request up into smaller parts? Otherwise, I recommend reviewing this PR as a whole instead of by commit because several of my earlier changes were reverted in later commits

Copy link
Member

@Techatrix Techatrix left a comment

Choose a reason for hiding this comment

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

Would it be preferred to break this pull request up into smaller parts? Otherwise, I recommend reviewing this PR as a whole instead of by commit because several of my earlier changes were reverted in later commits

For the future, create smaller PRs or try to structure the changes to be reviewable on a per-commit basis for larger PRs. At this scale it becomes hard to correlate code changes with changes in behavior and unit tests which is where looking at individual commits would be helpful.


Even though these issues are not caused by this PR, I figure I'd mention them here since they are related to generics and you may be interested in investigating them:

fn Foo() type {
    return struct {
        fn bar(self: @This()) void {
            _ = self;
        }
    };
}

test {
    var foo: Foo() = undefined;
}

Code completions for foo.bar do not exclude the self: @This() parameter. This appears to have been regressed some time between 0.13.0 and 0.14.0.

test {
    var set: @import("std").AutoArrayHashMapUnmanaged([3]u8, void) = .empty;
}

Code completions for set.values do not exclude the self parameter. This appears to have regressed in c105a37.

Copy link
Contributor Author

@FnControlOption FnControlOption left a comment

Choose a reason for hiding this comment

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

For the future, create smaller PRs or try to structure the changes to be reviewable on a per-commit basis for larger PRs. At this scale it becomes hard to correlate code changes with changes in behavior and unit tests which is where looking at individual commits would be helpful.

Okay, will do. What is your preference for updating a PR after you've reviewed it? For example, let's say I have some changes that could be squashed into an existing commit. Should I amend the commit and force push? Or should I create a new commit and push that?

fn Foo() type {
    return struct {
        fn bar(self: @This()) void {
            _ = self;
        }
    };
}

test {
    var foo: Foo() = undefined;
}

Code completions for foo.bar do not exclude the self: @This() parameter. This appears to have been regressed some time between 0.13.0 and 0.14.0.

I think this is caused by this line: https://github.com/zigtools/zls/blob/5b5dd6d/src/features/completions.zig#L348

Since the struct has no fields, isNamespace() returns true. It probably should also check if is_type_val == true

test {
    var set: @import("std").AutoArrayHashMapUnmanaged([3]u8, void) = .empty;
}

Code completions for set.values do not exclude the self parameter. This appears to have regressed in c105a37.

This appears to be fixed in this PR

@Techatrix
Copy link
Member

What is your preference for updating a PR after you've reviewed it? For example, let's say I have some changes that could be squashed into an existing commit. Should I amend the commit and force push? Or should I create a new commit and push that?

Modifying an existing commit to keep the commit history clean is generally ok since I can see the code changes of a force push. But in this PR, I am not sure why you would want to do that since I plan to squash merge this PR anyway. Just pushing new commits should suffice.

@FnControlOption

This comment was marked as outdated.

@Techatrix Techatrix merged commit bb6d647 into zigtools:master May 26, 2025
6 checks passed
@FnControlOption FnControlOption deleted the wip-generics branch May 26, 2025 01:31
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.

Semantic tokens don't work well with methods in generics variables with the type of a generic type parameter are resolved as type instead of T
2 participants