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

Add fix-it to error and add note - SR-4637 #8947

Conversation

@garricn
Copy link
Contributor

@garricn garricn commented Apr 23, 2017

This PR will add a removal fix-it to the ERROR attribute_requires_function_type when the @escaping attribute is applied to a non-function type input parameter.

It will also add the NOTE escaping_optional_type_argument to the same when the parameter is an optional closure.

See: https://twitter.com/garricn/status/854833614487379968

Resolves SR-4637.

@garricn
Copy link
Contributor Author

@garricn garricn commented Apr 23, 2017

@swift-ci please smoke test

@slavapestov
Copy link
Member

@slavapestov slavapestov commented Apr 23, 2017

You need commit access to trigger CI.

@swift-ci Please smoke test

@garricn
Copy link
Contributor Author

@garricn garricn commented Apr 23, 2017

@slavapestov Thank you for letting me know. I'll add that to the list of things I learned today :)

@garricn
Copy link
Contributor Author

@garricn garricn commented Apr 23, 2017

Well, looks like I need to take another look tomorrow.

@slavapestov
Copy link
Member

@slavapestov slavapestov commented Apr 23, 2017

You can run tests locally with 'build-script -t', or 'build-script -R -t' to build in release mode, which runs the tests much faster.

@garricn
Copy link
Contributor Author

@garricn garricn commented Apr 23, 2017

Three failures all for the same reason looks like:

Assertion failed: (isValid() && "Can't advance an invalid location"), function getAdvancedLoc, file /Users/PERSONAL/Developer/swift-source/swift/include/swift/Basic/SourceLoc.h, line 51

Must have something to do with my call to SourceRange I assume. I will have to look tomorrow. I am able to run tests with:

./llvm/utils/lit/lit.py -sv ./build/Ninja-RelWithDebInfoAssert/swift-macosx-x86_64/test-macosx-x86_64/ --filter=attr/[NAME OF FILE].swift

I will run all tests locally too.

Thank you!

UPDATE: I realize now that I failed to run all tests locally before pushing PR. My bad.

@garricn garricn force-pushed the garricn:add-error-and-fix-it-to-optional-closure-parameter-SR-4637 branch 2 times, most recently Apr 23, 2017
lib/Sema/TypeCheckType.cpp Outdated
TypeAttributes::getAttrName(i));
if (attrs.has(TAK_escaping)) {
auto &SM = TC.Context.SourceMgr;
auto loc = attrs.getLoc(TAK_escaping);

This comment has been minimized.

@slavapestov

slavapestov Apr 23, 2017
Member

Maybe you can just call attr.getRangeWithAt()?

This comment has been minimized.

@garricn

garricn Apr 29, 2017
Author Contributor

I think I would need to call getRangeWithAt() on an attr. How do I get the attr from attrs?

This comment has been minimized.

@CodaFi

CodaFi Apr 30, 2017
Collaborator

This entire file is using TypeAttributes, not DeclAttributes so the ugly way around is the only way around. It's ripe for refactoring.

lib/Sema/TypeCheckType.cpp Outdated
@@ -2221,8 +2221,21 @@ Type TypeResolver::resolveAttributedType(TypeAttributes &attrs,

for (auto i : FunctionAttrs) {
if (attrs.has(i)) {
TC.diagnose(attrs.getLoc(i), diag::attribute_requires_function_type,
TypeAttributes::getAttrName(i));
if (attrs.has(TAK_escaping)) {

This comment has been minimized.

@slavapestov

slavapestov Apr 23, 2017
Member

Indentation -- we use two space indent

This comment has been minimized.

@garricn

garricn Apr 29, 2017
Author Contributor

Noted. I use four space for my app projects. Any pro tip for quickly switching back and forth depending on the project?

This comment has been minimized.

@CodaFi

CodaFi Apr 29, 2017
Collaborator

Xcode has per-project indentation now.

lib/Sema/TypeCheckType.cpp Outdated
TC.diagnose(repr->getLoc(), diag::escaping_optional_type_argument);
}
} else {
TC.diagnose(attrs.getLoc(i), diag::attribute_requires_function_type,

This comment has been minimized.

@slavapestov

slavapestov Apr 23, 2017
Member

Can you unify this with the other case where we emit escaping_optional_type_argument, or extract the logic into a method? That code also has the getAdvancedLoc(-1)... business which could be replaced with getRangeWithAt().

@garricn garricn force-pushed the garricn:add-error-and-fix-it-to-optional-closure-parameter-SR-4637 branch to 4e27bbe Apr 30, 2017
@CodaFi
Copy link
Collaborator

@CodaFi CodaFi commented Apr 30, 2017

@swift-ci please smoke test

@CodaFi
Copy link
Collaborator

@CodaFi CodaFi commented Apr 30, 2017

Success!

Thanks @garricn, and welcome to the project.

⛵️

@CodaFi CodaFi merged commit 7863e29 into apple:master Apr 30, 2017
2 checks passed
2 checks passed
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details
@garricn
Copy link
Contributor Author

@garricn garricn commented Apr 30, 2017

Woohoo! Thank you so much for helping @CodaFi . I could not have done it without you. I learned a lot though. My ability to set my workstation with the Swift project, build it, run it, make changes, test the changes, and maintain my copy of the project has certainly increased, so thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.