Missing types in smodel #391
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refactored methods in `SemanticsHelpers.cs` to return tuples for eligibility checks, providing more detailed information. Updated various files to use the new tuple return type for member eligibility checks. Modified `CsPlainConstructorBuilder.cs` and `CsPlainSourceBuilder.cs` to store eligibility check results in variables before conditional logic. Updated `launchSettings.json` to change the `workingDirectory` path for the `ixc-simple-template` project configuration.
Updated method signatures in `SemanticsHelpers` to use `eligibleType` instead of `qualifiedName` for clarity and consistency. Enhanced `FindTypeDeclaration` method with additional null and type checks. Refactored multiple builder classes to consistently use `eligibleType`. Added or modified comments for future refactoring or debugging. Updated `launchSettings.json` to include `--skip-deps` argument for `ixc-simple-template` project configuration.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors type handling and eligibility checks in the CS Compiler project by updating method signatures to return tuples that include both a boolean eligibility flag and the corresponding type declaration, while also revising the code that consumes these methods. Key changes include:
- Updating methods in SemanticsHelpers.cs to return tuples (eligibility flag + type declaration) for members, variables, and arrays.
- Adapting Plain and Onliner source builders to use the new tuple format in eligibility checks and type handling.
- Bumping the version in GitVersion.yml to reflect the new changes.
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/AXSharp.compiler/src/AXSharp.Cs.Compiler/Helpers/SemanticsHelpers.cs | Changed method signatures to return tuples; added methods to resolve and determine fully qualified names. |
| src/AXSharp.compiler/src/AXSharp.Cs.Compiler/Plain/CsPlainSourceBuilder.cs | Modified element type resolution to use eligibility tuple from the new methods. |
| src/AXSharp.compiler/src/AXSharp.Cs.Compiler/Onliner/* | Updated eligibility checks and replaced direct type.Accept(visitor) calls with the tuple-based eligible type. |
| GitVersion.yml | Updated version from 0.23.0 to 0.24.0. |
Files not reviewed (1)
- src/AXSharp.compiler/src/ixc/Properties/launchSettings.json: Language not supported
Comments suppressed due to low confidence (2)
src/AXSharp.compiler/src/AXSharp.Cs.Compiler/Helpers/SemanticsHelpers.cs:61
- There is a redundant null-check for 'typeAccess' in the FindTypeDeclaration method; consider removing the duplicate check to streamline the logic.
if (typeAccess == null)
src/AXSharp.compiler/src/AXSharp.Cs.Compiler/Plain/CsPlainSourceBuilder.cs:131
- [nitpick] There are multiple commented-out calls to 'Accept(visitor, this)' that duplicate the new eligibility-based call; consider removing these legacy comments to keep the code clean.
//arrayType.ElementTypeAccess.Type.Accept(visitor, this);
src/AXSharp.compiler/src/AXSharp.Cs.Compiler/Helpers/SemanticsHelpers.cs
Outdated
Show resolved
Hide resolved
…Helpers.cs Co-authored-by: Copilot <[email protected]>
Corrected the property name 'isEligibe' to 'isEligible' across multiple files to ensure consistency and correctness. Changes include: - Updated return statement in `SemanticsHelpers.cs` to use `eligibility.isEligible`. - Updated condition checks in `CsOnlinerMemberBuilder.cs`, `CsOnlinerPlainerOnlineToPlainBuilder.cs`, `CsOnlinerPlainerPlainToOnlineBuilder.cs`, `CsOnlinerPlainerPlainToShadowBuilder.cs`, `CsOnlinerPlainerShadowToPlainBuilder.cs`, `CsOnlinerHasChangedBuilder.cs`, and `CsPlainSourceBuilder.cs` to use `eligibility.isEligible`.
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the eligibility checks in the AXSharp.Cs.Compiler project by updating method signatures to return tuples (eligibility status and type declaration) and by introducing new helper methods for resolving fully qualified type declarations. Key changes include:
- Changes in SemanticsHelpers.cs to return and propagate tuples instead of booleans.
- Updates in both Plain and Onliner source builder classes to use the new eligibility tuple and type resolution methods.
- A version bump in GitVersion.yml to reflect the new changes.
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Helpers/SemanticsHelpers.cs | Updated multiple eligibility methods to return (bool, ITypeDeclaration) tuples and added new type resolution helpers. |
| Plain/CsPlainSourceBuilder.cs | Updated field and variable declarations to use the new eligibility tuple along with modifications to type handling. |
| Onliner/CsOnlinerSourceBuilder.cs and related Onliner files | Refactored method calls and conditionals to utilize the eligibility tuple when constructing members and assignments. |
| GitVersion.yml | Bumped next-version from 0.23.0 to 0.24.0. |
| Onliner/CsOnlinerConstructorBuilder.cs, CsOnlinerMemberBuilder.cs, and others | Applied similar tuple-based eligibility checks in constructor and member builders. |
Files not reviewed (1)
- src/AXSharp.compiler/src/ixc/Properties/launchSettings.json: Language not supported
Comments suppressed due to low confidence (2)
src/AXSharp.compiler/src/AXSharp.Cs.Compiler/Plain/CsPlainSourceBuilder.cs:119
- The eligibility tuple returned by IsMemberEligibleForTranspile defines the property as 'isEligibe'; update the reference to 'eligibility.isEligibe' to prevent potential compilation errors.
if (eligibility.isEligible)
src/AXSharp.compiler/src/AXSharp.Cs.Compiler/Plain/CsPlainSourceBuilder.cs:131
- [nitpick] Consider removing or cleaning up commented-out code if it is no longer needed to improve code clarity.
//arrayType.ElementTypeAccess.Type.Accept(visitor, this);
Comment on lines
+110
to
+113
| var elibility = semantics.IsMemberEligibleForConstructor(SourceBuilder); | ||
| if (elibility.isEligibe) | ||
| { | ||
| switch (semantics.Type) | ||
| switch (elibility.eligibleType) |
There was a problem hiding this comment.
There is a typo in the variable name 'elibility'; consider renaming it to 'eligibility' for consistency and clarity.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request includes several changes to the
AXSharp.Cs.Compilerproject to enhance type handling and eligibility checks for transpilation. The most significant changes include updating method signatures to return tuples, adding new methods for determining fully qualified names, and modifying eligibility checks to use these new methods.Enhancements to type handling and eligibility checks:
src/AXSharp.compiler/src/AXSharp.Cs.Compiler/Helpers/SemanticsHelpers.cs: Updated multiple methods (IsMemberEligibleForTranspile,IsEligibleForTranspile,IsMemberEligibleForConstructor) to return tuples containing eligibility status and type declaration. Added methods to find type declarations and determine fully qualified names. [1] [2] [3] [4] [5] [6] [7] [8]src/AXSharp.compiler/src/AXSharp.Cs.Compiler/Onliner/CsOnlinerConfigurationConstructorBuilder.cs: UpdatedCreateVariableDeclarationandAddArrayMemberInitializationmethods to use the new eligibility check tuple format. [1] [2] [3]src/AXSharp.compiler/src/AXSharp.Cs.Compiler/Onliner/CsOnlinerConstructorBuilder.cs: ModifiedCreateFieldDeclarationandCreateVariableDeclarationmethods to switch on the eligible type returned by the new eligibility check methods. [1] [2]Version update:
GitVersion.yml: Updatednext-versionfrom0.23.0to0.24.0to reflect the new changes.