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

Introduce inst generic boundary #1192

Merged

Conversation

taichi-ishitani
Copy link
Contributor

@taichi-ishitani taichi-ishitani commented Jan 8, 2025

This PR is to a new generic boundary named inst.
The isnt boundary is to specify module or interface instance as a generic parameter.

refs: #963 (comment)

Copy link

codspeed-hq bot commented Jan 8, 2025

CodSpeed Performance Report

Merging #1192 will not alter performance

Comparing taichi-ishitani:interface_on_generic_args (33f9eb6) with master (b8588f5)

Summary

✅ 3 untouched benchmarks

@taichi-ishitani taichi-ishitani force-pushed the interface_on_generic_args branch 4 times, most recently from e7c90ff to 60cf6da Compare January 10, 2025 01:52
@@ -210,12 +210,62 @@ impl VerylGrammarTrait for CheckType<'_> {
));
}
}
GenericBoundKind::Inst(proto) => {
let proto_match = if arg.is_resolvable() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

At Rust 2024 (Rust 1.85), if-let chain will be stabilized.
After it, this large if chain can be re-written like below. So TODO comment may be useful to mark for re-writing.

if arg.is_resolvable()
  && let Ok(symbol) = ...
  && let SymbolKind::Instance(x) = ...
  && let Ok(x) = ...
  && let Some(ref x) = x.found.proto() {
  ...
} else {
  false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -41,3 +42,9 @@ module Module55H::<W: const> {

let _a: StructH::<W> = 0;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this example is bit strange.
IF is bound by Interface55I, but Interface55I is not proto but specific type, so IF is always Interface55I.
Therefore I think generics is not required in this case.
How about adding proto interface support described at #963 like below?

proto interface PInterface {
    function FuncA(a: input logic, b: input logic) -> logic;
}

interface Interface55I for PInterface {
    function FuncA(a: input logic, b: input logic) -> logic {
        return 0;
    }
}

module Module55I::<IF: PInterface> {
    inst u: IF;
}

module X {
    inst u: Module55I::<Interface55I>;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you but a lot of changes is needed to introduce proto interface.
For now, how about removing this example and opening a new PR to introduce proto interface?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree you. check_type.rs:387-390 and symbol.rs:304 should be removed probably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed changes to allow interface to be used as generic boundary.

@taichi-ishitani taichi-ishitani force-pushed the interface_on_generic_args branch 2 times, most recently from 59aa506 to c353e61 Compare January 14, 2025 01:53
@taichi-ishitani taichi-ishitani changed the title Enhance generic boundary Introduce inst generic boundary Jan 14, 2025
pub fn proto(&self) -> Option<SymbolPath> {
match &self.kind {
SymbolKind::Module(x) => x.proto.clone(),
SymbolKind::Interface(_) => Some((&self.token).into()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this line necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.
It is used at here.

if let Some(ref x) = symbol.found.proto() {

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this PR, interface can't be used as generic boudary. So I think adding SymbolKind::Interface(_) is not necessary, and SymbolKind::Interface(x) => x.proto.clone(), should be added at introducing proto interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The inst boundary with specific interface is needed for usecase shown in #793.
Therefore, this change is needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understood this line is used, but I think it is not appropriate that non proto symbol returns Some from proto().
Changing proto() to proto_or_inst() or splitting it into proto() and inst() is better.
I prefer the splitting way because inst can be assigned to a generic parameter bound as proto if proto and inst are not distincted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the statement for the SymbolKind::Interface but did not add the inst function.
Instead of the function, I added resolve_inst_generic_arg_type and resolve_proto_generic_arg_type functions to simplify resolving type of generic arguments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!
I think this is better than my idea.

@taichi-ishitani taichi-ishitani force-pushed the interface_on_generic_args branch 2 times, most recently from 314175c to 446f727 Compare January 15, 2025 05:01
* introduce a new generic bound `inst` to specify module/interface instance
@taichi-ishitani taichi-ishitani force-pushed the interface_on_generic_args branch from 446f727 to 33f9eb6 Compare January 15, 2025 05:07
@dalance dalance merged commit 6d3ff5a into veryl-lang:master Jan 15, 2025
9 checks passed
@taichi-ishitani taichi-ishitani deleted the interface_on_generic_args branch January 15, 2025 05:56
@dalance dalance added the enhancement New feature or request label Jan 20, 2025
@dalance dalance added this to the v0.13.5 milestone Jan 20, 2025
@dalance dalance mentioned this pull request Jan 31, 2025
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants