Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fixing a few edge cases around display name (+cleanup)#13552

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
dsyme merged 17 commits intomainfromcleanup/names
Aug 2, 2022

Conversation

dsyme
Copy link
Contributor

@dsymedsyme commentedJul 22, 2022
edited by psfinaki
Loading

fixes#13061
fixes#13593
fixes#13609
fixes#13610

The cleanup is related to logical names and display names, there are also some docs added.

@psfinakipsfinaki linked an issueJul 22, 2022 that may beclosed by this pull request
@auduchinok
Copy link
Member

@dsyme What do you think about normalizing names forlet bindings so they start with a lowercase letter? It'd match the core library and the most of F# code I've seen. I find the current uppercase names a bit problematic as tooling like Fantomas uses the uppercase/lowercase distinction to format applications differently to make method/constructor/union case invocations look more natural. We also try to go inline with this logic in our refactoring implementations.

I guess it should be discussed in the style guide repo instead and I'd be happy to move there, but it would be great to have these names follow the common conventions anyway.

@dsyme
Copy link
ContributorAuthor

@dsyme What do you think about normalizing names for let bindings so they start with a lowercase letter? It'd match the core library and the most of F# code I've seen. I find the current uppercase names a bit problematic as tooling like Fantomas uses the uppercase/lowercase distinction to format applications differently to make method/constructor/union case invocations look more natural. We also try to go inline with this logic in our refactoring implementations.

I can't really see us doing this. I understand the situation but across the major Checking and CodeGen and Optimization files I've found code clearer if the major recursive and/or revealed-in-signature routines (Gen*, Optimize*, Tc*) are capitalized, while utility functions from TypedTreeOps etc. are generally camelCase (though if functionality forms a logical unit, rather than utilities, they are sometimes PascalCase)

There's no clear dividing line for this and for PrettyNaming we're sort of caught in the middle - a somewhat messy set of lowerCase names in the implementation and the PascalCase ones in the implementation. In some ways the PascalCase indicates an intent to clean things up into a coherent whole.

@dsymedsyme changed the title[WIP] cleanup some things around namesCleanup some things around namesJul 29, 2022
@@ -35,6 +35,9 @@ These release notes track our current efforts to document changes to the F# proj
* `SynMeasure` was extended with [SynMeasure.Paren](https://fsharp.github.io/fsharp-compiler-docs/reference/fsharp-compiler-syntax-synmeasure.html#Paren) case.
* Dynamic expressions (like `x?y`) are now represented as [SynExpr.Dynamic](https://fsharp.github.io/fsharp-compiler-docs/reference/fsharp-compiler-syntax-synexpr.html#Dynamic) in the Untyped Syntax Tree.
* Members with `get` and/or `set` are now represented as [SynMemberDefn.GetSetMember](https://fsharp.github.io/fsharp-compiler-docs/reference/fsharp-compiler-syntax-synmemberdefn.html#GetSetMember) in the Untyped Syntax Tree.
* `DoesIdentifierNeedBackticks` is removed, it should always be sufficient to call `NormalizeIdentifierBackticks` or else call something in `PrettyNaming`
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@auduchinok@baronfel Can you check this? We think it is OK but would like to know if Rider or VSCode are depending on these

Copy link
Member

Choose a reason for hiding this comment

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

So there will be followups on that PR, if we find issues we can revert some of these changes in those followups

Copy link
Member

@auduchinokauduchinokAug 18, 2022
edited
Loading

Choose a reason for hiding this comment

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

@dsyme It seems it won't affect us. I'll raise an issue if anything doesn't work as expected.

Thanks for double-checking it for other tools! 🙂

@dsyme
Copy link
ContributorAuthor

@psfinaki So this is looking good, I'm glad to see it green

I'm surprised that no other tests failed. There are quite a lot of places where I believe we will have fixed bugs, e.g. quick info for active pattern names that contain spaces,let (|``Thing with space``|_) x = ... and also other similar caes.

Should we do a pass together where we review the code and characterise these in test cases? I'll go through the code now and mark up bugs I believe fixed

thanks
don

@dsyme
Copy link
ContributorAuthor

dsyme commentedJul 29, 2022
edited
Loading

I notice that we didn't end up fixing the problems withfullName* andFullNameOfItem that stem from uses ofEnclosingPath andEnclosingMangledPath (both of which should be renamed toEnclosingLogicalPath). For example quick info for hovering overx in

module``Thingwith space``=letx=1

where double-ticks are not added to the full path reported. Similarly hovering overx here is showing theAModule logical name mangling:

typeA=classendmoduleA=letx=1

Could you list this as a separate bug? I'll write some notes about how to fix it:

  • Basically all thefullName* functions need to be duplicated out to be eitherfullLogicalName* orfullDisplayName* with appropriate implementations to get back the display name along the path.
  • However this is a little tricky for the *Module names which need to switch to use theCompilationPath instead of thePublicPath to pick up theModuleOrNamespaceKind which indicate if module name mangling happened.

thanks

@dsyme
Copy link
ContributorAuthor

If you could add test cases for the above that would be great. Once that's done this is ready to go :)

@psfinakipsfinaki changed the titleCleanup some things around namesFixing a few edge cases around display name (+cleanup)Jul 29, 2022
@psfinaki
Copy link
Member

I notice that we didn't end up fixing the problems withfullName* andFullNameOfItem that stem from uses ofEnclosingPath andEnclosingMangledPath (both of which should be renamed toEnclosingLogicalPath). For example quick info for hovering overx in

module``Thingwith space``=letx=1

where double-ticks are not added to the full path reported. Similarly hovering overx here is showing theAModule logical name mangling:

typeA=classendmoduleA=letx=1

Could you list this as a separate bug? I'll write some notes about how to fix it:

  • Basically all thefullName* functions need to be duplicated out to be eitherfullLogicalName* orfullDisplayName* with appropriate implementations to get back the display name along the path.
  • However this is a little tricky for the *Module names which need to switch to use theCompilationPath instead of thePublicPath to pick up theModuleOrNamespaceKind which indicate if module name mangling happened.

thanks

@dsyme Yep, created followup issues:#13613,#13614

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@auduchinokauduchinokauduchinok left review comments

@psfinakipsfinakipsfinaki approved these changes

@KevinRansomKevinRansomKevinRansom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@dsyme@auduchinok@psfinaki@KevinRansom

[8]ページ先頭

©2009-2025 Movatter.jp