Skip to content

Conversation

@ntovas
Copy link
Contributor

@ntovas ntovas commented Oct 10, 2022

Most likely there is a better way to do this, any comment appreciated

Fixes: #13377

PS: I am a c# dev, it's my first time with f#, I would like to implement better (SynConst.String(c.ToString(), SynStringKind.Regular, m)) but I dont know how

@ntovas ntovas changed the title Infer string type when using nameof pattern #13377 Infer string type when using nameof pattern Oct 10, 2022
@T-Gro
Copy link
Member

T-Gro commented Oct 11, 2022

Hi, had a look at the failing checks:

https://dev.azure.com/dnceng-public/public/_build/results?buildId=46781&view=logs&j=966222a4-f29d-5ec0-88cc-1b798728906e&t=052eabf8-67e2-5f29-745c-00cc34aa0e02 is hinting two failing tests related to this change:

at FSharp.Tests.Core.VersionTests.nameof-fsi() in D:\a_work\1\s\tests\fsharp\tests.fs:line 2102

and

at FSharp.Tests.Core.VersionTests.nameof-execute() in D:\a_work\1\s\tests\fsharp\tests.fs:line 2099

This should bring you to test cases where a new regression was introduced, please try to untackle this.
If it does not work, let me know and I can have a look later.

@ntovas
Copy link
Contributor Author

ntovas commented Oct 11, 2022

@T-Gro
The tests seem to fail at PatternMatchingWithNameof, more specifically on the function:

let deserialize (e: RecordedEvent) : MyEvent =
        match e.EventType with
        | nameof A -> A e.Data
        | nameof B -> B e.Data
        | t -> failwithf "Invalid EventType: %s" t

Unfortunately, when I am running the same code with VisualFSharpDebug with targets: .net 7.0 - .net 4.7.2 it works as expected.
Am I missing something?

@T-Gro
Copy link
Member

T-Gro commented Oct 17, 2022

How did you run the same code please?
Depending on the test project, there might be "special" ways of launching the test suite - best to follow the readme.md in the /tests folder.

Or, alternatively, run Build.cmd with respective -test*** switch, this is what CI does.
(each CI report contains the cmd arguments used to launch it)

@ntovas
Copy link
Contributor Author

ntovas commented Oct 17, 2022

Yes, I did that and in my local env it also fails.
I copied the same code in order to debug it and find the issue, so my main question is, how can I debug this case to find what is going wrong? I found no way to debug these test cases.

@T-Gro
Copy link
Member

T-Gro commented Oct 17, 2022

Yes, I did that and in my local env it also fails. I copied the same code in order to debug it and find the issue, so my main question is, how can I debug this case to find what is going wrong? I found no way to debug these test cases.

This specific is calling dotnet.exe with build /t:RunFSharpScript arguments => running a .fsx file.

@ntovas
Copy link
Contributor Author

ntovas commented Oct 18, 2022

Ok, thanks, I look into this.

Update: Unfortunately I cannot solve this, the test continues to fail, but all the nameof resolutions seems to produce the correct const string. I don't have the required knowledge to find what is wrong :(
@T-Gro do you want me to close this MR?

@ntovas ntovas force-pushed the ntovas/fix-nameof-pattern branch from 8d51296 to e759f26 Compare October 18, 2022 19:06
@T-Gro
Copy link
Member

T-Gro commented Oct 20, 2022

Ok, thanks, I look into this.

Update: Unfortunately I cannot solve this, the test continues to fail, but all the nameof resolutions seems to produce the correct const string. I don't have the required knowledge to find what is wrong :( @T-Gro do you want me to close this MR?

Please keep it, I do want to have this working too.
I will try to start on it next milestone/month, and check how to troubleshoot that error.

I would love to help more, but from purely looking at it I do not have any ideas nor tips.

@T-Gro T-Gro self-assigned this Oct 20, 2022
@T-Gro T-Gro added this to the Backlog milestone Oct 20, 2022
@T-Gro T-Gro modified the milestones: Backlog, November-2022 Nov 2, 2022
@T-Gro T-Gro requested a review from a team as a code owner November 3, 2022 14:36
@T-Gro
Copy link
Member

T-Gro commented Nov 3, 2022

I think I got the issue now, will submit soon.

The Const.ToString() is a debug-oriented view, and therefore adds quotes around the string when rendering it.
That meant that within patterns, a nameof A was actually replaced with "A" , incl. the quotes in the string itself.

Which then meant that the pattern did not match.

I have also replaced the code to be more strict and insist on Const.String, since nameof should not produce any other constant type besides a string.

Fixing nameof pattern matching by avoiding .ToString(), which is a debug-oriented representation that adds double quotes to the string.

Which then meant a difference between string itself and then one being matched.

Also adding a case to the component tests suite to cover for this.
@psfinaki
Copy link
Contributor

psfinaki commented Nov 3, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

@ntovas thanks for the great first contribution :)
(and @T-Gro thanks for help)

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

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Compiler does not infer string type for nameof pattern

4 participants