Conversation
…teFromLongIdent helper Co-authored-by: 7sharp9 <588746+7sharp9@users.noreply.github.com>
SynType.CreateFromLongIdent helper
There was a problem hiding this comment.
Pull request overview
This PR reduces duplication in the built-in generator plugins by centralizing the repeated LongIdent -> SynType construction into a shared SynType.CreateFromLongIdent helper in Myriad.Core.
Changes:
- Added
SynType.CreateFromLongIdent : LongIdent -> SynTypetoMyriad.Core/AstExtensions.fs. - Updated
DUCasesGenerator.fs,LensesGenerator.fs, andFieldsGenerator.fsto use the new helper instead of local duplicated bindings. - Reformatted
.paket/Paket.Restore.targets(large whitespace/indentation churn).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Myriad.Core/AstExtensions.fs | Introduces SynType.CreateFromLongIdent helper for generators to reuse. |
| src/Myriad.Plugins/DUCasesGenerator.fs | Replaces repeated duType construction with SynType.CreateFromLongIdent. |
| src/Myriad.Plugins/LensesGenerator.fs | Replaces repeated recordType/duType construction with SynType.CreateFromLongIdent. |
| src/Myriad.Plugins/FieldsGenerator.fs | Replaces repeated recordType construction with SynType.CreateFromLongIdent. |
| .paket/Paket.Restore.targets | Large formatting-only change unrelated to the refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static member CreateFromLongIdent (parent: LongIdent) = | ||
| SynLongIdent.Create (parent |> List.map (fun i -> i.idText)) | ||
| |> SynType.CreateLongIdent |
There was a problem hiding this comment.
SynType.CreateFromLongIdent currently rebuilds the LongIdent via idText -> Ident.Create, which discards the existing Ident ranges from parent and does extra allocations. Since this file already defines SynLongIdent.CreateFromLongIdent (longIdent: LongIdent), consider implementing this as parent |> SynLongIdent.CreateFromLongIdent |> SynType.CreateLongIdent to preserve source ranges and reuse the existing helper.
| <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <!-- Prevent dotnet template engine to parse this file --> | ||
| <!--/-:cnd:noEmit--> | ||
| <PropertyGroup> | ||
| <!-- make MSBuild track this file for incremental builds. --> | ||
| <!-- ref https://blogs.msdn.microsoft.com/msbuild/2005/09/26/how-to-ensure-changes-to-a-custom-target-file-prompt-a-rebuild/ --> | ||
| <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects> | ||
|
|
||
| <DetectedMSBuildVersion>$(MSBuildVersion)</DetectedMSBuildVersion> | ||
| <DetectedMSBuildVersion Condition="'$(MSBuildVersion)' == ''">15.0.0</DetectedMSBuildVersion> | ||
| <MSBuildSupportsHashing>false</MSBuildSupportsHashing> | ||
| <MSBuildSupportsHashing Condition=" '$(DetectedMSBuildVersion)' > '15.8.0' ">true</MSBuildSupportsHashing> | ||
| <!-- Mark that this target file has been loaded. --> | ||
| <IsPaketRestoreTargetsFileLoaded>true</IsPaketRestoreTargetsFileLoaded> | ||
| <PaketToolsPath>$(MSBuildThisFileDirectory)</PaketToolsPath> | ||
| <PaketRootPath>$(MSBuildThisFileDirectory)..\</PaketRootPath> | ||
| <PaketRestoreCacheFile>$(PaketRootPath)paket-files\paket.restore.cached</PaketRestoreCacheFile> | ||
| <PaketLockFilePath>$(PaketRootPath)paket.lock</PaketLockFilePath> | ||
| <PaketBootstrapperStyle>classic</PaketBootstrapperStyle> | ||
| <PaketBootstrapperStyle Condition="Exists('$(PaketToolsPath)paket.bootstrapper.proj')">proj</PaketBootstrapperStyle> | ||
| <PaketExeImage>assembly</PaketExeImage> | ||
| <PaketExeImage Condition=" '$(PaketBootstrapperStyle)' == 'proj' ">native</PaketExeImage> | ||
| <MonoPath Condition="'$(MonoPath)' == '' AND Exists('/Library/Frameworks/Mono.framework/Commands/mono')">/Library/Frameworks/Mono.framework/Commands/mono</MonoPath> | ||
| <MonoPath Condition="'$(MonoPath)' == ''">mono</MonoPath> | ||
|
|
||
| <!-- PaketBootStrapper --> | ||
| <PaketBootStrapperExePath Condition=" '$(PaketBootStrapperExePath)' == '' AND Exists('$(PaketRootPath)paket.bootstrapper.exe')">$(PaketRootPath)paket.bootstrapper.exe</PaketBootStrapperExePath> | ||
| <PaketBootStrapperExePath Condition=" '$(PaketBootStrapperExePath)' == '' ">$(PaketToolsPath)paket.bootstrapper.exe</PaketBootStrapperExePath> | ||
| <PaketBootStrapperExeDir Condition=" Exists('$(PaketBootStrapperExePath)') " >$([System.IO.Path]::GetDirectoryName("$(PaketBootStrapperExePath)"))\</PaketBootStrapperExeDir> | ||
|
|
||
| <PaketBootStrapperCommand Condition=" '$(OS)' == 'Windows_NT' ">"$(PaketBootStrapperExePath)"</PaketBootStrapperCommand> | ||
| <PaketBootStrapperCommand Condition=" '$(OS)' != 'Windows_NT' ">$(MonoPath) --runtime=v4.0.30319 "$(PaketBootStrapperExePath)"</PaketBootStrapperCommand> | ||
|
|
||
| <!-- Disable automagic references for F# DotNet SDK --> | ||
| <!-- This will not do anything for other project types --> | ||
| <!-- see https://github.com/fsharp/fslang-design/blob/master/tooling/FST-1002-fsharp-in-dotnet-sdk.md --> | ||
| <DisableImplicitFSharpCoreReference>true</DisableImplicitFSharpCoreReference> | ||
| <DisableImplicitSystemValueTupleReference>true</DisableImplicitSystemValueTupleReference> | ||
|
|
||
| <!-- Disable Paket restore under NCrunch build --> | ||
| <PaketRestoreDisabled Condition="'$(NCrunch)' == '1'">True</PaketRestoreDisabled> | ||
|
|
||
| <!-- Disable test for CLI tool completely - overrideable via properties in projects or via environment variables --> | ||
| <PaketDisableCliTest Condition=" '$(PaketDisableCliTest)' == '' ">False</PaketDisableCliTest> | ||
|
|
||
| <PaketIntermediateOutputPath Condition=" '$(PaketIntermediateOutputPath)' == '' ">$(BaseIntermediateOutputPath.TrimEnd('\').TrimEnd('\/'))</PaketIntermediateOutputPath> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- Resolve how paket should be called --> | ||
| <!-- Current priority is: local (1: repo root, 2: .paket folder) => 3: as CLI tool => as bootstrapper (4: proj Bootstrapper style, 5: BootstrapperExeDir) => 6: global path variable --> | ||
| <Target Name="SetPaketCommand" > | ||
| <!-- Test if paket is available in the standard locations. If so, that takes priority. Case 1/2 - non-windows specific --> | ||
| <PropertyGroup Condition=" '$(OS)' != 'Windows_NT' "> | ||
| <!-- no windows, try native paket as default, root => tool --> | ||
| <PaketExePath Condition=" '$(PaketExePath)' == '' AND Exists('$(PaketRootPath)paket') ">$(PaketRootPath)paket</PaketExePath> | ||
| <PaketExePath Condition=" '$(PaketExePath)' == '' AND Exists('$(PaketToolsPath)paket') ">$(PaketToolsPath)paket</PaketExePath> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- Test if paket is available in the standard locations. If so, that takes priority. Case 2/2 - same across platforms --> | ||
| <PropertyGroup> | ||
| <!-- root => tool --> | ||
| <PaketExePath Condition=" '$(PaketExePath)' == '' AND Exists('$(PaketRootPath)paket.exe') ">$(PaketRootPath)paket.exe</PaketExePath> | ||
| <PaketExePath Condition=" '$(PaketExePath)' == '' AND Exists('$(PaketToolsPath)paket.exe') ">$(PaketToolsPath)paket.exe</PaketExePath> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- If paket hasn't be found in standard locations, test for CLI tool usage. --> | ||
| <!-- First test: Is CLI configured to be used in "dotnet-tools.json"? - can result in a false negative; only a positive outcome is reliable. --> | ||
| <PropertyGroup Condition=" '$(PaketExePath)' == '' "> | ||
| <_DotnetToolsJson Condition="Exists('$(PaketRootPath)/.config/dotnet-tools.json')">$([System.IO.File]::ReadAllText("$(PaketRootPath)/.config/dotnet-tools.json"))</_DotnetToolsJson> | ||
| <_ConfigContainsPaket Condition=" '$(_DotnetToolsJson)' != ''">$(_DotnetToolsJson.Contains('"paket"'))</_ConfigContainsPaket> | ||
| <_ConfigContainsPaket Condition=" '$(_ConfigContainsPaket)' == ''">false</_ConfigContainsPaket> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- Second test: Call 'dotnet paket' and see if it returns without an error. Mute all the output. Only run if previous test failed and the test has not been disabled. --> | ||
| <!-- WARNING: This method can lead to processes hanging forever, and should be used as little as possible. See https://github.com/fsprojects/Paket/issues/3705 for details. --> | ||
| <Exec Condition=" '$(PaketExePath)' == '' AND !$(PaketDisableCliTest) AND !$(_ConfigContainsPaket)" Command="dotnet paket --version" IgnoreExitCode="true" StandardOutputImportance="low" StandardErrorImportance="low" > | ||
| <Output TaskParameter="ExitCode" PropertyName="LocalPaketToolExitCode" /> | ||
| </Exec> | ||
|
|
||
| <!-- If paket is installed as CLI use that. Again, only if paket haven't already been found in standard locations. --> | ||
| <PropertyGroup Condition=" '$(PaketExePath)' == '' AND ($(_ConfigContainsPaket) OR '$(LocalPaketToolExitCode)' == '0') "> | ||
| <_PaketCommand>dotnet paket</_PaketCommand> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- If neither local files nor CLI tool can be found, final attempt is searching for boostrapper config before falling back to global path variable. --> | ||
| <PropertyGroup Condition=" '$(PaketExePath)' == '' AND '$(_PaketCommand)' == '' "> | ||
| <!-- Test for bootstrapper setup --> | ||
| <PaketExePath Condition=" '$(PaketExePath)' == '' AND '$(PaketBootstrapperStyle)' == 'proj' ">$(PaketToolsPath)paket</PaketExePath> | ||
| <PaketExePath Condition=" '$(PaketExePath)' == '' AND Exists('$(PaketBootStrapperExeDir)') ">$(PaketBootStrapperExeDir)paket</PaketExePath> | ||
|
|
||
| <!-- If all else fails, use global path approach. --> | ||
| <PaketExePath Condition=" '$(PaketExePath)' == ''">paket</PaketExePath> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- If not using CLI, setup correct execution command. --> | ||
| <PropertyGroup Condition=" '$(_PaketCommand)' == '' "> | ||
| <_PaketExeExtension>$([System.IO.Path]::GetExtension("$(PaketExePath)"))</_PaketExeExtension> | ||
| <_PaketCommand Condition=" '$(_PaketCommand)' == '' AND '$(_PaketExeExtension)' == '.dll' ">dotnet "$(PaketExePath)"</_PaketCommand> | ||
| <_PaketCommand Condition=" '$(_PaketCommand)' == '' AND '$(OS)' != 'Windows_NT' AND '$(_PaketExeExtension)' == '.exe' ">$(MonoPath) --runtime=v4.0.30319 "$(PaketExePath)"</_PaketCommand> | ||
| <_PaketCommand Condition=" '$(_PaketCommand)' == '' ">"$(PaketExePath)"</_PaketCommand> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- The way to get a property to be available outside the target is to use this task. --> | ||
| <CreateProperty Value="$(_PaketCommand)"> | ||
| <Output TaskParameter="Value" PropertyName="PaketCommand"/> | ||
| </CreateProperty> | ||
|
|
||
| </Target> | ||
|
|
||
| <Target Name="PaketBootstrapping" Condition="Exists('$(PaketToolsPath)paket.bootstrapper.proj')"> | ||
| <MSBuild Projects="$(PaketToolsPath)paket.bootstrapper.proj" Targets="Restore" /> | ||
| </Target> | ||
|
|
||
| <!-- Official workaround for https://docs.microsoft.com/en-us/visualstudio/msbuild/getfilehash-task?view=vs-2019 --> | ||
| <UsingTask TaskName="Microsoft.Build.Tasks.GetFileHash" AssemblyName="Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" Condition=" '$(MSBuildSupportsHashing)' == 'true' And '$(DetectedMSBuildVersion)' < '16.0.360' " /> | ||
| <UsingTask TaskName="Microsoft.Build.Tasks.VerifyFileHash" AssemblyName="Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" Condition=" '$(MSBuildSupportsHashing)' == 'true' And '$(DetectedMSBuildVersion)' < '16.0.360' " /> | ||
| <Target Name="PaketRestore" Condition="'$(PaketRestoreDisabled)' != 'True'" BeforeTargets="_GenerateDotnetCliToolReferenceSpecs;_GenerateProjectRestoreGraphPerFramework;_GenerateRestoreGraphWalkPerFramework;CollectPackageReferences" DependsOnTargets="SetPaketCommand;PaketBootstrapping"> | ||
|
|
||
| <!-- Step 1 Check if lockfile is properly restored (if the hash of the lockfile and the cache-file match) --> | ||
| <PropertyGroup> | ||
| <PaketRestoreRequired>true</PaketRestoreRequired> | ||
| <NoWarn>$(NoWarn);NU1603;NU1604;NU1605;NU1608</NoWarn> | ||
| <CacheFilesExist>false</CacheFilesExist> | ||
| <CacheFilesExist Condition=" Exists('$(PaketRestoreCacheFile)') And Exists('$(PaketLockFilePath)') ">true</CacheFilesExist> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- Read the hash of the lockfile --> | ||
| <GetFileHash Condition=" '$(MSBuildSupportsHashing)' == 'true' And '$(CacheFilesExist)' == 'true' " Files="$(PaketLockFilePath)" Algorithm="SHA256" HashEncoding="hex" > | ||
| <Output TaskParameter="Hash" PropertyName="PaketRestoreLockFileHash" /> | ||
| </GetFileHash> | ||
| <!-- Read the hash of the cache, which is json, but a very simple key value object --> | ||
| <PropertyGroup Condition=" '$(MSBuildSupportsHashing)' == 'true' And '$(CacheFilesExist)' == 'true' "> | ||
| <PaketRestoreCachedContents>$([System.IO.File]::ReadAllText('$(PaketRestoreCacheFile)'))</PaketRestoreCachedContents> | ||
| </PropertyGroup> | ||
| <ItemGroup Condition=" '$(MSBuildSupportsHashing)' == 'true' And '$(CacheFilesExist)' == 'true' "> | ||
| <!-- Parse our simple 'paket.restore.cached' json ...--> | ||
| <PaketRestoreCachedSplitObject Include="$([System.Text.RegularExpressions.Regex]::Split(`$(PaketRestoreCachedContents)`, `{|}|,`))"></PaketRestoreCachedSplitObject> | ||
| <!-- Keep Key, Value ItemGroup--> | ||
| <PaketRestoreCachedKeyValue Include="@(PaketRestoreCachedSplitObject)" | ||
| Condition=" $([System.Text.RegularExpressions.Regex]::Split(`%(Identity)`, `": "`).Length) > 1 "> | ||
| <Key>$([System.Text.RegularExpressions.Regex]::Split(`%(Identity)`, `": "`)[0].Replace(`"`, ``).Replace(` `, ``))</Key> | ||
| <Value>$([System.Text.RegularExpressions.Regex]::Split(`%(Identity)`, `": "`)[1].Replace(`"`, ``).Replace(` `, ``))</Value> | ||
| </PaketRestoreCachedKeyValue> | ||
| </ItemGroup> | ||
| <PropertyGroup Condition=" '$(MSBuildSupportsHashing)' == 'true' And '$(CacheFilesExist)' == 'true' "> | ||
| <!-- Retrieve the hashes we are interested in --> | ||
| <PackagesDownloadedHash Condition=" '%(PaketRestoreCachedKeyValue.Key)' == 'packagesDownloadedHash' ">%(PaketRestoreCachedKeyValue.Value)</PackagesDownloadedHash> | ||
| <ProjectsRestoredHash Condition=" '%(PaketRestoreCachedKeyValue.Key)' == 'projectsRestoredHash' ">%(PaketRestoreCachedKeyValue.Value)</ProjectsRestoredHash> | ||
| </PropertyGroup> | ||
|
|
||
| <PropertyGroup Condition=" '$(MSBuildSupportsHashing)' == 'true' And '$(CacheFilesExist)' == 'true' "> | ||
| <!-- If the restore file doesn't exist we need to restore, otherwise only if hashes don't match --> | ||
| <PaketRestoreRequired>true</PaketRestoreRequired> | ||
| <PaketRestoreRequired Condition=" '$(PaketRestoreLockFileHash)' == '$(ProjectsRestoredHash)' ">false</PaketRestoreRequired> | ||
| <PaketRestoreRequired Condition=" '$(PaketRestoreLockFileHash)' == '' ">true</PaketRestoreRequired> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- | ||
| <!-- Prevent dotnet template engine to parse this file --> | ||
| <!--/-:cnd:noEmit--> | ||
| <PropertyGroup> | ||
| <!-- make MSBuild track this file for incremental builds. --> | ||
| <!-- ref https://blogs.msdn.microsoft.com/msbuild/2005/09/26/how-to-ensure-changes-to-a-custom-target-file-prompt-a-rebuild/ --> | ||
| <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects> |
There was a problem hiding this comment.
This change reformats a large portion of .paket/Paket.Restore.targets (indentation/whitespace) without an apparent functional change and outside the PR’s stated purpose. Because this file is typically managed by Paket and is imported by multiple projects, it’s best to revert whitespace-only edits here to reduce diff noise and avoid future merge conflicts when Paket regenerates it.
The 3-line pattern for converting a
LongIdentto aSynTypewas copy-pasted 9 times acrossDUCasesGenerator.fs,LensesGenerator.fs, andFieldsGenerator.fs, creating a maintenance hazard.Changes
Myriad.Core/AstExtensions.fs— AddedSynType.CreateFromLongIdentalongside the existingSynTypeextension methods:DUCasesGenerator.fs— Replaced 4 occurrences (createToString,createFromString,createToTag,createIsCase)LensesGenerator.fs— Replaced 3 occurrences (createLensForRecordField,createLensForDU×2)FieldsGenerator.fs— Replaced 2 occurrences (createFieldMap,createCreate)Original prompt
duTypeConstruction fromparentLongIdent in Generator Plugins #193💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.