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

Review usages of Display/WithEngines for CallPath and provide dedicated to_xyz_string methods instead #6873

Open
ironcev opened this issue Jan 31, 2025 · 1 comment
Labels
compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler General compiler. Should eventually become more specific as the issue is triaged

Comments

@ironcev
Copy link
Member

ironcev commented Jan 31, 2025

Display and DisplayWithEngines for CallPath simply concatenate the call path prefixes and the suffix, without being aware of any contextual information, like, if the path is a full path, or an ambiguos one, or for which purpose is the string representation of the path requested.

This is a caveat because it leads to generation of paths which are invalid in a particular context. E.g., the string representation of the full path [package, mod_a, mod_b, Item] within the package is ::mod_a::mod_b::Item and not package::mod_a::mod_b::Item.

Note that Display and DisplayWithEngines do not have access to the namespace within which the path is observed and thus cannot display path with the complete contextual information.

This is error-prone because methods like to_string_with_arg in #6871 can unintentionally use Display to get the path which will in many cases look like the expected path, but will be, e.g., lacking leading :: when needed.

Also, we have cases like this one where the callers are burdend with writing cumbersome code to decide if they should put :: as a prefix or not.

We need to review the usages of Display and DisplayWithEngines for CallPath to check what kind of a path is actually expected in those usages, in respect to the CallPathType and the current namespace and module.

The proposal is to add dedicated to_xyz_string methods to CallPath that will accept namespace parameter and return the string properly formated to the context and usage. E.g., to_full_use_path_string.

The proposal is also, to fully remove the Display and DisplayWithEngines implementations and leave only Debug and DebugWithEngines. That way we can avoid unintentional error-prone usage and force the usage of the intended to_xyz_string method. Removal might be an issue though, as Display and DisplayWithEngines might be called recurively inside of other Display implementations. Here, I see a general question of the semantic correctness of such "parent" Display/WithEngine implementations.

@ironcev ironcev added compiler General compiler. Should eventually become more specific as the issue is triaged compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen labels Jan 31, 2025
@jjcnn
Copy link
Contributor

jjcnn commented Jan 31, 2025

Additionally, we have cases in the namespace module where we convert module paths to strings. The code that performs that conversion is very similar to the code used in the CallPath implementations of Display, so we can probably eliminate some code duplication as part of this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler General compiler. Should eventually become more specific as the issue is triaged
Projects
None yet
Development

No branches or pull requests

2 participants