chore(jsii): better errors around use of hidden types #1861
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
RomainMuller
added
effort/small
|
Added another commit to actually switch the way we test for errors in the "negatives" to actually look at the rendered error entirely (thanks to jest snapshots) in order to have a better picture of the error UX we are offering. Marked |
RomainMuller
added
the
pr/no-squash
rix0rrr
approved these changes
Aug 10, 2020
njlynch
suggested changes
Aug 10, 2020
| @@ -1,5 +1,3 @@ | |||
| ///!MATCH_ERROR: Interface contains behavior: name should be "ISomething" | |||
There was a problem hiding this comment.
I'm not seeing this error (or equivalent) in the snapshot. Am I missing it, or did this get dropped somehow?
There was a problem hiding this comment.
😨 OMG good catch. I guess I was never asserting there is > 0 problems found...
There was a problem hiding this comment.
Oh and the issue turned out to be essentially that location-less problem markers are NOT rendered with the API style I was going for... Dun dun dun!
When a hidden (that is, `@internal` or not exported) type is used in a visible (`public` or `protected` on an exported type) API, the error produced would refer to the unusable type, but would not give any indication of where it was being used from. This makes several enhancements to this process: - Qualify the kind of use for the type (return, parameter, ...) - Attach the error to the resolving node (usage location) - Provide a related message with the unusable type's declaration - Specifically message around "this" (used or inferred as a return type) This is going to particularly enhance the experience of folks extending internal base types, where those internal base types declare members that return hidden types (or "this"). Fixes #1860
Makes it easier to review the actual, complete, user experience (including error locality).
RomainMuller
force-pushed
the
rmuller/better-errors-reutrning-this-from-internal-type
branch
from
baeebbe to
d7d1d1b
Compare
RomainMuller
added
pr/do-not-merge
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
njlynch
approved these changes
Aug 10, 2020
RomainMuller
deleted the
rmuller/better-errors-reutrning-this-from-internal-type
branch
RomainMuller
added a commit
that referenced
this pull request
Aug 10, 2020
Using `--project-references`, nested types from dependencies could not be used as they would result in the following `jsii` error: ``` Type "<type name>" cannot be used as the return type because it is private or @internal ``` THis is because in `--project-references` mode, the ambient declaration for the nested type is the declaration that gets resolved to, and those cannot have the `export` keyword: instead, the surrounding module declaration is annotated with `export ambient`. This adds the necessary code path to actually identify this scenario and to appropriately detect the type is actually visible and exported. The check that was failing had been added in #1861 as a way to provide a more helpful error message when private or `@internal` types are mistakenly used on exported APIs; which explains why this situation did not previously occur.
RomainMuller
added a commit
that referenced
this pull request
Aug 12, 2020
Using `--project-references`, nested types from dependencies could not be used as they would result in the following `jsii` error: ``` Type "<type name>" cannot be used as the return type because it is private or @internal ``` This is because in `--project-references` mode, the ambient declaration for the nested type is the declaration that gets resolved to, and those cannot have the `export` keyword: instead, the surrounding module declaration is annotated with `export ambient`. This adds the necessary code path to actually identify this scenario and to appropriately detect the type is actually visible and exported. The check that was failing had been added in #1861 as a way to provide a more helpful error message when private or `@internal` types are mistakenly used on exported APIs; which explains why this situation did not previously occur. > Hidden gem: we were previously adding the `static` keyword when > generating nested classes in **C#**. This is a **Java**-ism and results > in code that won't compile, since **C#** does not allow `static` nested > classes to declare `protected` members (which we actually emit some > of).
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.
When a hidden (that is,
@internalor not exported) type is used in avisible (
publicorprotectedon an exported type) API, the errorproduced would refer to the unusable type, but would not give any
indication of where it was being used from.
This makes several enhancements to this process:
This is going to particularly enhance the experience of folks extending
internal base types, where those internal base types declare members
that return hidden types (or "this").
Fixes #1860
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.