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

[swift] Add macro expansion support for swift 5.9 #4698

Merged
merged 1 commit into from Dec 11, 2023

Conversation

kenji21
Copy link
Contributor

@kenji21 kenji21 commented Sep 29, 2023

Describe the PR

Introduce support for Swift 5.9, with new macros feature

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by github actions)
  • Added (in-code) documentation (if needed)

@kenji21 kenji21 changed the base branch from master to pmd/6.5.x September 29, 2023 12:40
@kenji21
Copy link
Contributor Author

kenji21 commented Sep 29, 2023

don't know on which branch I should PR into

@jsotuyod
Copy link
Member

@kenji21 PRs should be submitted against master to be included on the next release.

@oowekyala oowekyala changed the base branch from pmd/6.5.x to master September 29, 2023 12:50
Copy link
Member

@jsotuyod jsotuyod left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

My understanding is that Swift 5.9 actually included more language features, such as the ability to use if and switch as expressions for variable assignment and return values:

statusBar.text = if !hasConnection { "Disconnected" }
                 else if let error = lastError { error.localizedDescription }
                 else { "Ready" }

We should ensure this is supported before declaring 5.9 as a supported / default language version.

@kenji21
Copy link
Contributor Author

kenji21 commented Sep 29, 2023

Thanks for the PR.

My understanding is that Swift 5.9 actually included more language features, such as the ability to use if and switch as expressions for variable assignment and return values:

statusBar.text = if !hasConnection { "Disconnected" }
                 else if let error = lastError { error.localizedDescription }
                 else { "Ready" }

We should ensure this is supported before declaring 5.9 as a supported / default language version.

Ok, it is now, since I initially added the Swift5.9.swift file on a branch starting from the 6.55 tag, which has the "old path for SwiftTokenizerTest.java" https://github.com/pmd/pmd/blob/pmd_releases/6.55.0/pmd-swift/src/test/java/net/sourceforge/pmd/cpd/SwiftTokenizerTest.java#L61 file which references swift all files to test
My rebase didn't find it... in the lang/swift subdirectories => pmd-swift/src/test/java/net/sourceforge/pmd/lang/swift/cpd/SwiftTokenizerTest.java

@kenji21 kenji21 force-pushed the feature/swift-5.9-support branch 2 times, most recently from be9fbbc to 1b84e0c Compare September 29, 2023 13:57
@kenji21
Copy link
Contributor Author

kenji21 commented Sep 29, 2023

oh... rebasing from 6.55 to master isn't enough...

@kenji21
Copy link
Contributor Author

kenji21 commented Oct 5, 2023

A little help on this failure is welcomed:

[INFO] Running net.sourceforge.pmd.lang.swift.ast.SwiftParserTests
[INFO]     └─ ✘ testSimpleSwift
[INFO]     └─ ✘ testBtree
[INFO] Tests run: 2, Failures: 1, Errors: 1, Skipped: 0, Time elapsed: 0.189 s <<< FAILURE! - in net.sourceforge.pmd.lang.swift.ast.SwiftParserTests
[ERROR] net.sourceforge.pmd.lang.swift.ast.SwiftParserTests.testSimpleSwift  Time elapsed: 0.153 s  <<< FAILURE!
java.lang.AssertionError: Token of kind 78 has no XPath name (literal '#')
	at net.sourceforge.pmd.lang.ast.impl.antlr4.AntlrNameDictionary.<init>(AntlrNameDictionary.java:70)
	at net.sourceforge.pmd.lang.swift.ast.SwiftNameDictionary.<init>(SwiftNameDictionary.java:17)
	at net.sourceforge.pmd.lang.swift.ast.SwiftParser.<clinit>(SwiftParser.java:339)
	at net.sourceforge.pmd.lang.swift.ast.PmdSwiftParser.parse(PmdSwiftParser.java:21)
	at net.sourceforge.pmd.lang.swift.ast.PmdSwiftParser.parse(PmdSwiftParser.java:17)
	at net.sourceforge.pmd.lang.ast.impl.antlr4.AntlrBaseParser.parse(AntlrBaseParser.java:30)
	at net.sourceforge.pmd.lang.ast.impl.antlr4.AntlrBaseParser.parse(AntlrBaseParser.java:22)
	at net.sourceforge.pmd.lang.ast.test.BaseParsingHelper.parseImpl(BaseParsingHelper.kt:157)
	at net.sourceforge.pmd.lang.ast.test.BaseParsingHelper.doParse(BaseParsingHelper.kt:143)
	at net.sourceforge.pmd.lang.ast.test.BaseParsingHelper.parse(BaseParsingHelper.kt:132)
	at net.sourceforge.pmd.lang.ast.test.BaseParsingHelper.parse$default(BaseParsingHelper.kt:122)
	at net.sourceforge.pmd.lang.ast.test.BaseTreeDumpTest$doTest$1.invoke(BaseTreeDumpTest.kt:37)
	at net.sourceforge.pmd.lang.ast.test.BaseTreeDumpTest$doTest$1.invoke(BaseTreeDumpTest.kt:35)
	at net.sourceforge.pmd.test.BaseTextComparisonTest.doTest$pmd_lang_test(BaseTextComparisonTest.kt:46)
	at net.sourceforge.pmd.lang.ast.test.BaseTreeDumpTest.doTest(BaseTreeDumpTest.kt:35)
	at net.sourceforge.pmd.lang.ast.test.BaseTreeDumpTest.doTest$default(BaseTreeDumpTest.kt:34)
	at net.sourceforge.pmd.lang.ast.test.BaseTreeDumpTest.doTest(BaseTreeDumpTest.kt)
	at net.sourceforge.pmd.lang.swift.ast.SwiftParserTests.testSimpleSwift(SwiftParserTests.java:13)
	[...]

for some context, swift 5.9 has macros:

let x = 3.14159
let y = 2.71828
#myMacro(x + y)

So I added only few lines to have them in the Swift.g4 file:

// GRAMMAR OF A MACRO DECLARATION

macroDeclaration: '#' identifier ;

@adangel adangel added an:enhancement An improvement on existing features / rules in:grammar About the grammar of a lexer or parser, eg, a parse/lex exception labels Nov 10, 2023
@adangel adangel changed the title Add swift 5.9 support [swift] Add swift 5.9 support Nov 10, 2023
@adangel
Copy link
Member

adangel commented Nov 10, 2023

@kenji21 Sorry for the late response.

The current antlr integration in PMD is quite straight: we end up actually using the parser tree from antlr and are not creating an abstract syntax tree (which is on the roadmap for later). That's why terminal symbols like '#' end up in the PMD's "AST" representation.

For such terminal symbols, we need to provide a name, so that these can be queried with XPath expressions. This is defined in

protected @Nullable String nonAlphaNumName(String name) {
and there we need to add a case for '#', e.g.

case "#": return "hash";

Then this exception should be solved.

@adangel adangel changed the title [swift] Add swift 5.9 support [swift] Add macro expansion support for swift 5.9 Nov 10, 2023
@kenji21 kenji21 force-pushed the feature/swift-5.9-support branch 2 times, most recently from c24931a to a7ce5d1 Compare November 13, 2023 14:53
@kenji21
Copy link
Contributor Author

kenji21 commented Nov 14, 2023

rebased on current master, CI now fails on pmd-dist :

  [INFO] PMD Swift .......................................... SUCCESS [ 22.868 s]
  [INFO] PMD TSql ........................................... SUCCESS [  9.878 s]
  [INFO] PMD Visualforce .................................... SUCCESS [ 17.538 s]
  [INFO] PMD Velocity ....................................... SUCCESS [ 13.989 s]
  [INFO] PMD XML and XSL .................................... SUCCESS [ 12.410 s]
  [INFO] PMD Languages Dependencies ......................... SUCCESS [  1.798 s]
  [INFO] PMD Documentation Generator ........................ SUCCESS [ 12.880 s]
  [INFO] PMD Scala - Transitional package (deprecated) ...... SUCCESS [  0.796 s]
  [INFO] PMD Scala for Scala 2.12 ........................... SUCCESS [ 20.589 s]
  [INFO] PMD CLI ............................................ SUCCESS [ 12.160 s]
  [INFO] PMD Distribution Packages .......................... FAILURE [01:20 min]

@adangel
Copy link
Member

adangel commented Nov 14, 2023

2023-11-13T15:09:35.3729479Z [ERROR] Failures: 
2023-11-13T15:09:35.3729846Z [ERROR]   BinaryDistributionIT.testPmdHelp:156 
2023-11-13T15:09:35.3738645Z Expected: a string matching the pattern <.*Validvalues:\Qapex-52\E,\Qapex-53\E,\Qapex-54\E,\Qapex-55\E,\Qapex-56\E,\Qapex-57\E,\Qapex-58\E,\Qapex-59\E,\Qecmascript-3\E,\Qecmascript-5\E,\Qecmascript-6\E,\Qecmascript-7\E,\Qecmascript-8\E,\Qecmascript-9\E,\Qecmascript-ES2015\E,\Qecmascript-ES2016\E,\Qecmascript-ES2017\E,\Qecmascript-ES2018\E,\Qecmascript-ES6\E,\Qhtml-4\E,\Qhtml-5\E,\Qjava-1.10\E,\Qjava-1.3\E,\Qjava-1.4\E,\Qjava-1.5\E,\Qjava-1.6\E,\Qjava-1.7\E,\Qjava-1.8\E,\Qjava-1.9\E,\Qjava-10\E,\Qjava-11\E,\Qjava-12\E,\Qjava-13\E,\Qjava-14\E,\Qjava-15\E,\Qjava-16\E,\Qjava-17\E,\Qjava-18\E,\Qjava-19\E,\Qjava-20\E,\Qjava-20-preview\E,\Qjava-21\E,\Qjava-21-preview\E,\Qjava-5\E,\Qjava-6\E,\Qjava-7\E,\Qjava-8\E,\Qjava-9\E,\Qjsp-2\E,\Qjsp-3\E,\Qkotlin-1.6\E,\Qkotlin-1.7\E,\Qkotlin-1.8\E,\Qmodelica-3.4\E,\Qmodelica-3.5\E,\Qplsql-11g\E,\Qplsql-12.1\E,\Qplsql-12.2\E,\Qplsql-12c_Release_1\E,\Qplsql-12c_Release_2\E,\Qplsql-18c\E,\Qplsql-19c\E,\Qplsql-21c\E,\Qpom-4.0.0\E,\Qscala-2.10\E,\Qscala-2.11\E,\Qscala-2.12\E,\Qscala-2.13\E,\Qswift-4.2\E,\Qswift-5.0\E,\Qswift-5.1\E,\Qswift-5.2\E,\Qswift-5.3\E,\Qswift-5.4\E,\Qswift-5.5\E,\Qswift-5.6\E,\Qswift-5.7\E,\Qvf-52\E,\Qvf-53\E,\Qvf-54\E,\Qvf-55\E,\Qvf-56\E,\Qvf-57\E,\Qvf-58\E,\Qvf-59\E,\Qvm-2.0\E,\Qvm-2.1\E,\Qvm-2.2\E,\Qvm-2.3\E,\Qwsdl-1.1\E,\Qwsdl-2.0\E,\Qxml-1.0\E,\Qxml-1.1\E,\Qxsl-1.0\E,\Qxsl-2.0\E,\Qxsl-3.0\E.*>
2023-11-13T15:09:35.3786720Z      but: the string was "Usage:pmdcheck[-bDh][--no-cache][--[no-]fail-on-violation][--[no-]progress][--no-ruleset-compatibility][--show-suppressed][--aux-classpath=<auxClasspath>][--cache=<cacheLocation>][-e=<encoding>][-f=<format>][--file-list=<fileListPath>][--force-language=<forceLanguage>][--ignore-list=<ignoreListPath>][--minimum-priority=<minimumPriority>][-r=<reportFile>][--suppress-marker=<suppressMarker>][-t=<threads>][-u=<uri>][-P=<String=String>]...[--use-version=<languageVersion>]...[-d=<inputPaths>[,<inputPaths>...]...]...-R=<rulesets>[,<rulesets>...]...[-R=<rulesets>[,<rulesets>...]...]...[-z=<relativizePathsWith>[,<relativizePathsWith>...]...]...[<inputPaths>...]ThePMDstandardsourcecodeanalyzer[<inputPaths>...]Pathtoasourcefile,ordirectorycontainingsourcefilestoanalyze.Equivalenttousing--dir.--aux-classpath=<auxClasspath>Specifiestheclasspathforlibrariesusedbythesourcecode.ThisisusedtoresolvetypesinJavasourcefiles.Theplatformspecificpathdelimiter(\":\"onLinux,\";\"onWindows)isusedtoseparatetheentries.Alternatively,asingle'file:'URLtoatextfilecontainingpathelementsonconsecutivelinescanbespecified.-b,--benchmarkBenchmarkmode-outputabenchmarkreportuponcompletion;defaulttoSystem.err.--cache=<cacheLocation>Specifythelocationofthecachefileforincrementalanalysis.Thisshouldbethefullpathtothefile,includingthedesiredfilename(notjusttheparentdirectory).Ifthefiledoesn'texist,itwillbecreatedonthefirstrun.Thefilewillbeoverwrittenoneachrunwiththemostup-to-dateruleviolations.-d,--dir=<inputPaths>[,<inputPaths>...]...Pathtoasourcefile,ordirectorycontainingsourcefilestoanalyze.ZipandJarfilesarealsosupported,iftheyarespecifieddirectly(archivefilesfoundwhileexploringadirectoryarenotrecursivelyexpanded).Thisoptioncanberepeated,andmultipleargumentscanbeprovidedtoasingleoccurrenceoftheoption.Oneof--dir,--file-listor--urimustbeprovided.-D,-v,--debug,--verboseDebugmode.-e,--encoding=<encoding>SpecifiesthecharactersetencodingofthesourcecodefilesDefault:UTF-8-f,--format=<format>Reportformat.Validvalues:codeclimate,csv,emacs,empty,html,ideaj,json,sarif,summaryhtml,text,textcolor,textpad,vbhtml,xml,xslt,yahtmlAlternatively,youcanprovidethefullyqualifiednameofacustomRendererintheclasspath.Default:text--file-list=<fileListPath>Pathtoafilecontainingalistoffilestoanalyze,onepathperline.Oneof--dir,--file-listor--urimustbeprovided.--force-language=<forceLanguage>Forcealanguagetobeusedforallinputfiles,irrespectiveoffilenames.Whenusingthisoption,theautomaticlanguageselectionbyextensionisdisabled,andPMDtriestoparseallinputfileswiththegivenlanguage'sparser.Parsingerrorsareignored.Validvalues:apex,ecmascript,html,java,jsp,kotlin,modelica,plsql,pom,scala,swift,vf,vm,wsdl,xml,xsl-h,--helpShowthishelpmessageandexit.--ignore-list=<ignoreListPath>Pathtoafilecontainingalistoffilestoexcludefromtheanalysis,onepathperline.Thisoptioncanbecombinedwith--dir,--file-listand--uri.--minimum-priority=<minimumPriority>Ruleprioritythreshold;ruleswithlowerprioritythanconfiguredherewon'tbeused.Validvalues(caseinsensitive):HIGH,MEDIUM_HIGH,MEDIUM,MEDIUM_LOW,LOWDefault:Low--no-cacheExplicitlydisableincrementalanalysis.The'-cache'optionisignoredifthisswitchispresentinthecommandline.--[no-]fail-on-violationBydefaultPMDexitswithstatus4ifviolationsarefound.Disablethisoptionwith'--no-fail-on-violation'toexitwith0insteadandjustwritethereport.--[no-]progressEnables/disablesprogressbarindicatorofliveanalysisprogress.--no-ruleset-compatibilityDisabletherulesetcompatibilityfilter.Thefilterisactivebydefaultandtriesautomatically'fix'oldrulesetfileswitholdrulenames-P,--property=<String=String>Key-valuepairdefiningapropertyforthereportformat.Supportedvaluesforeachreportformat:csv:problem-IncludeProblemcolumnDefault:truepackage-IncludePackagecolumnDefault:truefile-IncludeFilecolumnDefault:truepriority-IncludePrioritycolumnDefault:trueline-IncludeLinecolumnDefault:truedesc-IncludeDescriptioncolumnDefault:trueruleSet-IncludeRulesetcolumnDefault:truerule-IncludeRulecolumnDefault:truehtml:linkPrefix-PathtoHTMLsource.linePrefix-Prefixforlinenumberanchorinthesourcefile.Default:Optional.emptyhtmlExtension-Replacefileextensionwith.htmlforthelinks.Default:falseideaj:fileName-Filename.sourcePath-Sourcepath.classAndMethodName-ClassandMethodname,pass'.method'whenprocessingadirectory.summaryhtml:linkPrefix-PathtoHTMLsource.linePrefix-Prefixforlinenumberanchorinthesourcefile.Default:Optional.emptyhtmlExtension-Replacefileextensionwith.htmlforthelinks.Default:falsetextcolor:color-Enablescolorswithanythingotherthan'false'or'0'.Default:yesxml:encoding-XMLencodingformatDefault:UTF-8xslt:encoding-XMLencodingformatDefault:UTF-8xsltFilename-TheXSLTfilename.yahtml:outputDir-Outputdirectory.Default:.-r,--report-file=<reportFile>Pathtoafiletowhichreportoutputiswritten.Thefileiscreatedifitdoesnotexist.Ifthisoptionisnotspecified,thereportisrenderedtostandardoutput.-R,--rulesets=<rulesets>[,<rulesets>...]...Pathtoarulesetxmlfile.Thepathmayreferencearesourceontheclasspathoftheapplication,bealocalfilesystempath,oraURL.Theoptioncanberepeated,andmultipleargumentsseparatedbycommacanbeprovidedtoasingleoccurrenceoftheoption.--show-suppressedReportshouldshowsuppressedruleviolations.--suppress-marker=<suppressMarker>SpecifiesthestringthatmarksalinewhichPMDshouldignore.Default:NOPMD-t,--threads=<threads>SetsthenumberofthreadsusedbyPMD.Default:1-u,--uri=<uri>DatabaseURIforsources.Oneof--dir,--file-listor--urimustbeprovided.--use-version=<languageVersion>ThelanguageversionPMDshouldusewhenparsingsourcecode.Validvalues:apex-52,apex-53,apex-54,apex-55,apex-56,apex-57,apex-58,apex-59,ecmascript-3,ecmascript-5,ecmascript-6,ecmascript-7,ecmascript-8,ecmascript-9,ecmascript-ES2015,ecmascript-ES2016,ecmascript-ES2017,ecmascript-ES2018,ecmascript-ES6,html-4,html-5,java-1.10,java-1.3,java-1.4,java-1.5,java-1.6,java-1.7,java-1.8,java-1.9,java-10,java-11,java-12,java-13,java-14,java-15,java-16,java-17,java-18,java-19,java-20,java-20-preview,java-21,java-21-preview,java-5,java-6,java-7,java-8,java-9,jsp-2,jsp-3,kotlin-1.6,kotlin-1.7,kotlin-1.8,modelica-3.4,modelica-3.5,plsql-11g,plsql-12.1,plsql-12.2,plsql-12c_Release_1,plsql-12c_Release_2,plsql-18c,plsql-19c,plsql-21c,pom-4.0.0,scala-2.10,scala-2.11,scala-2.12,scala-2.13,swift-4.2,swift-5.0,swift-5.1,swift-5.2,swift-5.3,swift-5.4,swift-5.5,swift-5.6,swift-5.7,swift-5.8,swift-5.9,vf-52,vf-53,vf-54,vf-55,vf-56,vf-57,vf-58,vf-59,vm-2.0,vm-2.1,vm-2.2,vm-2.3,wsdl-1.1,wsdl-2.0,xml-1.0,xml-1.1,xsl-1.0,xsl-2.0,xsl-3.0-z,--relativize-paths-with=<relativizePathsWith>[,<relativizePathsWith>...]...Pathrelativetowhichdirectoriesarerenderedinthereport.Thisoptionallowsshorteningdirectoriesinthereport;withoutit,pathsarerenderedasmentionedinthesourcedirectory(option\"--dir\").Theoptioncanberepeated,inwhichcasetheshortestrelativepathwillbeused.Iftherootpathismentioned(e.g.\"/\"or\"C:\\\"),thenthepathswillberenderedasabsolute."

That's expected, because you added a new language version. You need to fix the integration test:

"swift-4.2", "swift-5.0", "swift-5.1", "swift-5.2",
"swift-5.3", "swift-5.4", "swift-5.5", "swift-5.6",
"swift-5.7", "vf-52", "vf-53", "vf-54", "vf-55", "vf-56",

@pmd-test
Copy link

pmd-test commented Nov 19, 2023

1 Message
📖 Compared to master:
This changeset changes 0 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.
Download full report as build artifact
Compared to master:
This changeset changes 0 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.
Download full report as build artifact

Generated by 🚫 Danger

@kenji21
Copy link
Contributor Author

kenji21 commented Nov 20, 2023

I was trying to locally test a release on real source code, but ./mvnw clean verify doesn't make a zip in pmd-dist/target dir

@adangel
Copy link
Member

adangel commented Nov 21, 2023

I was trying to locally test a release on real source code, but ./mvnw clean verify doesn't make a zip in pmd-dist/target dir

You'll need to run ./mvnw clean verify -Pcli-dist to enable the profile "cli-dist", which builds the zip. This will be fixed at some point (see #4746)

@kenji21 kenji21 force-pushed the feature/swift-5.9-support branch 2 times, most recently from a671bbe to b423336 Compare December 1, 2023 13:48
@adangel adangel added this to the 7.0.0 milestone Dec 11, 2023
Copy link
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

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

Thanks!

I'll update the grammar a bit (e.g. to parse the macro expansion expression as an expression rather than a declaration) and add parser tests in net.sourceforge.pmd.lang.swift.ast.SwiftParserTests to make sure, we actually support macro expansions.

@adangel adangel merged commit 10ae2fa into pmd:master Dec 11, 2023
3 checks passed
adangel added a commit that referenced this pull request Dec 11, 2023
adangel added a commit that referenced this pull request Dec 11, 2023
[swift] Add macro expansion support for swift 5.9 #4698
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
an:enhancement An improvement on existing features / rules in:grammar About the grammar of a lexer or parser, eg, a parse/lex exception
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[swift] Support Swift 5.9 features (mainly macros expansion expressions)
4 participants