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

miss a method to access all PathType in PathSegment #15778

Closed
Young-Flash opened this issue Oct 18, 2023 · 4 comments · Fixed by #15875
Closed

miss a method to access all PathType in PathSegment #15778

Young-Flash opened this issue Oct 18, 2023 · 4 comments · Fixed by #15875
Assignees
Labels
C-feature Category: feature request

Comments

@Young-Flash
Copy link
Member

Young-Flash commented Oct 18, 2023

There are only pub fn path_type(&self) -> Option<PathType> for PathSegment, which only return the first PathType, but there maybe more than one PathType in a PathSegment, take <i32 as std::ops::Add> for example, the AST of it is as the following:

PATH_SEGMENT@48..70
  L_ANGLE@48..49 "<"
  PATH_TYPE@49..52
    ... // i32
  WHITESPACE@52..53 " "
  AS_KW@53..55 "as"
  WHITESPACE@55..56 " "
  PATH_TYPE@56..69
    ... // std::ops::Add
  R_ANGLE@69..70 ">"

So I think there can make a pub fn path_types(&self) -> Vec<PathType> { support::children(&self.syntax).collect() } for PathSegment to access all PathType.

@Young-Flash Young-Flash added the C-feature Category: feature request label Oct 18, 2023
@Veykril
Copy link
Member

Veykril commented Oct 18, 2023

Interesting, our grammar is also wrong here

| '<' PathType ('as' PathType)? '>'

That should be

| '<' Type ('as' PathType)? '>'

and then we should implement the path type accessor manually given the potential overlap, somewhat similar to

impl ast::Impl {
pub fn self_ty(&self) -> Option<ast::Type> {
match self.target() {
(Some(t), None) | (_, Some(t)) => Some(t),
_ => None,
}
}
pub fn trait_(&self) -> Option<ast::Type> {
match self.target() {
(Some(t), Some(_)) => Some(t),
_ => None,
}
}
fn target(&self) -> (Option<ast::Type>, Option<ast::Type>) {
let mut types = support::children(self.syntax());
let first = types.next();
let second = types.next();
(first, second)
}
pub fn for_trait_name_ref(name_ref: &ast::NameRef) -> Option<ast::Impl> {
let this = name_ref.syntax().ancestors().find_map(ast::Impl::cast)?;
if this.trait_()?.syntax().text_range().start() == name_ref.syntax().text_range().start() {
Some(this)
} else {
None
}
}
}

@Young-Flash
Copy link
Member Author

Should we modify the syntax tree structure of PathSegment (seems a breaking change)? search the codebase but can't figure out where parse the i32 of <i32 as std::ops::Add> into a PathType, any suggestion?

According to PathSegment = ... | '<' Type ('as' PathType)? '>', there should be only one PathType within a PathSegment, right?
If so, can we just simplely make PathType accessor as (this is what I do here basically):

pub fn path_type(&self) -> Option<PathType> { 
    support::children(&self.syntax).last()
}

fn path_type has no usage in the codebase currently, so it will not raise breaking change.

@Veykril
Copy link
Member

Veykril commented Nov 10, 2023

The parser already parses a Type instead of a PathType, so it's just that the grammar is wrong which we need to fix up. The reason why we can't just make a path_type accessor like you say is that the Type in in the grammar rule could be a PathType as well, in which case the path_type accessor will yield the wrong one. That's why that special handling is required.

@Young-Flash
Copy link
Member Author

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants