Skip to content

Keep track of file package scope(user project, library, compiler, etc.) - #2031

Merged
Timothee Guerin (timotheeguerin) merged 22 commits into
microsoft:mainfrom
timotheeguerin:feature/scoped-files
Jun 16, 2023
Merged

Keep track of file package scope(user project, library, compiler, etc.)#2031
Timothee Guerin (timotheeguerin) merged 22 commits into
microsoft:mainfrom
timotheeguerin:feature/scoped-files

Conversation

@timotheeguerin

@timotheeguerin Timothee Guerin (timotheeguerin) commented Jun 7, 2023

Copy link
Copy Markdown
Member

This is a key piece to ensure a linter can omit warnings in non project code.

The resolution goes as follow:

  • start in project scope
  • if loading a directory or library that has a package.json set scope to that library. If library loads another library update the scope to be the nested library.
  • when loading compiler files set scope to compiler

Questions to answer:

  • Do we need to expose that on the type or have an helper to see what are the scope(s) of a type

@github-actions

github-actions Bot commented Jun 7, 2023

Copy link
Copy Markdown
Contributor

Changes in this PR will be published to the following url to try(check status of TypeSpec Pull Request Try It pipeline for publish status):
Playground: https://cadlplayground.z22.web.core.windows.net/prs/2031/

Website: https://cadlwebsite.z1.web.core.windows.net/prs/2031/

Comment thread packages/compiler/core/program.ts Outdated
});
});

describe("if a file is imported from main project and library, FIRST loaded wins...", () => {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the current behavior. It is not very nice to not have consistency here but at the same time this might be sign of a bad design from project structure.

Alternative would be to always promote to the project scope if it is part of it in any way

@timotheeguerin Timothee Guerin (timotheeguerin) changed the title [WIP] Keep track of file package scope(user project, library, compiler, etc.) Keep track of file package scope(user project, library, compiler, etc.) Jun 12, 2023
@timotheeguerin
Timothee Guerin (timotheeguerin) marked this pull request as ready for review June 12, 2023 16:28
Comment thread packages/compiler/core/program.ts Outdated
@bterlson

Copy link
Copy Markdown
Member

Overall, the notion of hooking module resolution to establish a map of sourceFile -> "project" | "compiler" | "library" feels good.

Some thoughts:

  1. I'm not sure I like using the word "scope" here. Like, the scope of a type is quite literally the container it is declared in. Consider something more descriptive? (Will also ponder).
  2. Should be an API that takes a type and returns its scope
  3. Need to figure out what to do with types which don't correspond to AST nodes, e.g. "virtual" types constructed via projections. I think there are cases where the node property on types is undefined.

@nguerrera

Copy link
Copy Markdown
Contributor

"Domain"?

@bterlson

Copy link
Copy Markdown
Member

I like domain, ish? But considering "Realms" is coming, "domains" and "realms" might be a bit much? Or we go all in and have features named "kingdoms", "fiefdoms", etc.? :-D

Could also just be super descriptive - LocationContext or something like that?

@timotheeguerin

Copy link
Copy Markdown
Member Author

I tried naming it DeclarationDomain but yeah DeclarationContext or LocationContext could work.

@nguerrera

Copy link
Copy Markdown
Contributor

That works for me too.

(Separately, my mind is wandering, thinking about a new name for the whole project where we lean into having "kingdoms" and "fiefdoms".)

@timotheeguerin

Copy link
Copy Markdown
Member Author

dom establish .

ally "@typespec/http";

trade Typespec.Http;

kingdom MyService.Foo {
  knight Foo {
     knighHaveProperties: string;
  }
}

@bterlson

Copy link
Copy Markdown
Member

Timothee Guerin (@timotheeguerin) Ok but this is actually really good! I think we have our April Fools joke for next year?? Could pretty easily be supported 😈

My issue with DeclarationX is that I think we will want to pass non-declarations to this, e.g. template instantiations especially, which should have the fiefdom of where it is instantiated rather than where it is declared, probably.

@timotheeguerin

Copy link
Copy Markdown
Member Author

The template instantiation is a good point, I think it would be a different helper but resulting context object would be the same so Declaration is indeed not a good name. I guess LocationContext is ok, just feel quite generic for what Location is

@timotheeguerin

Copy link
Copy Markdown
Member Author

Brian Terlson (@bterlson) Nick Guerrera (@nguerrera) could I get rereview of this PR, would like to be able to use that in the linter PR #2066

Comment thread packages/compiler/src/core/program.ts Outdated
Comment thread packages/compiler/src/core/types.ts
@timotheeguerin
Timothee Guerin (timotheeguerin) merged commit 9baa7d2 into microsoft:main Jun 16, 2023
@timotheeguerin
Timothee Guerin (timotheeguerin) deleted the feature/scoped-files branch June 16, 2023 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants