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

feat(versioning): add code fixes for incompatible version errors #5459

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

archerzz
Copy link
Member

@archerzz archerzz commented Dec 27, 2024

  • add code fixes for incompatible version errors
  • fix inconsistent error messages
  • add unit test cases
  • remove redundant codes

resolve #4842

- add code fixes for most of incompatible version errors
- fix inconsistent error message formats
- add unit test case
- remove redudant codes

resolve microsoft#4842
@@ -208,13 +209,6 @@ export function $onValidate(program: Program) {
validateVersionedNamespaceUsage(program, namespaceDependencies);
}

function getAllVersions(p: Program, t: Type): Version[] | undefined {
Copy link
Member Author

@archerzz archerzz Dec 27, 2024

Choose a reason for hiding this comment

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

it's already defined in versioning.ts:

function getAllVersions(p: Program, t: Type): Version[] | undefined {

@azure-sdk
Copy link
Collaborator

azure-sdk commented Dec 27, 2024

All changed packages have been documented.

  • @typespec/versioning
Show changes

@typespec/versioning - feature ✏️

add code fixes for incompatible version errors

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs 🛝 VSCode Extension

@RodgeFu RodgeFu added the ide Issues for VS, VSCode, Monaco, etc. label Dec 27, 2024
Mingzhe Huang (from Dev Box) added 2 commits December 30, 2024 10:06
@@ -799,8 +798,7 @@ function validateAvailabilityForRef(
targetAvail: Map<string, Availability>,
source: Type,
target: Type,
sourceOptions?: TypeNameOptions,
Copy link
Member Author

Choose a reason for hiding this comment

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

those two parameters are never really passed in, so I delete them for simplicity

@@ -162,7 +162,7 @@ describe("versioning: validate incompatible references", () => {
expectDiagnostics(diagnostics, {
code: "@typespec/versioning/incompatible-versioned-reference",
message:
"'TestService.test' is referencing versioned type 'TestService.Foo' but is not versioned itself.",
Copy link
Member Author

@archerzz archerzz Dec 30, 2024

Choose a reason for hiding this comment

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

looks like we don't compare the error message in unit testing, but I update them anyway

@archerzz
Copy link
Member Author

A few issues worth mentioning.

Fix Parameter Versions

I cannot insert @added(Version.V3) with a space in the following case:

@added(Versions.v2)
model Foo {}

op test(param: Foo): void;

Ideally, it should be op test(@added(Version.V2) param:Foo): void;. But actually, the fix is:

op test(@added(Version.V2)
param:Foo): void;

The reason is that the type of param is ModelProperty, not FunctionParameter. So, I don't know if the type is a model or a parameter. For models, we should insert a decorator with newline. And for parameters, we should insert a decorator with a space. To maintain maximum compatibility, I choose to insert the decorator with a new line.

Fix by Insertion

In this PR, I only provide fixes to insert @added and @removed. However, if you check those test cases, you'll realize that we could fix by deleting or replacing existing decorators. However, I didn't find an easy and safe way to delete or replace, so I choose to only insert, which in some cases will look stupid thought it still works.

@added(Versions.V3)
@added(Versions.V2) // a better fix is to replace `@added(Versions.V3)`
model Foo {};
@removed(Versions.V2)
@added(Versions.V2) // a better fix is to delete `@removed(Versions.V2)`
model Foo{};

Alternative fixes

Some error cases can have other ways to fix. But that requires more context which is not available in the validation logic, unless we do more changes. For example,

@added(Versions.v3)
model Foo {}
        
model Bar {
  foo: Foo;
}

The error is reported on Bar.foo. We can fix by adding @added(Versions.v2) to Bar.foo. Or we can add the same decorator to Bar. I choose to fix in where the error is reported to minimize the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ide Issues for VS, VSCode, Monaco, etc. lib:versioning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Codefix that add missing versioning annotations
3 participants